Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context
On Mon 12-12-22 09:06:10, Richard Guy Briggs wrote:
This patch adds a flag, FAN_INFO and an extensible buffer to provide
additional information about response decisions. The buffer contains
one or more headers defining the information type and the length of the
following information. The patch defines one additional information
type, FAN_RESPONSE_INFO_AUDIT_RULE, to audit a rule number. This will
allow for the creation of other information types in the future if other
users of the API identify different needs.
Suggested-by: Steve Grubb <sgrubb(a)redhat.com>
Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
Suggested-by: Jan Kara <jack(a)suse.cz>
Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com>
Thanks for the patches. They look very good to me. Just two nits below. I
can do the small updates on commit if there would be no other changes. But
I'd like to get some review from audit guys for patch 3/3 before I commit
this.
diff --git a/fs/notify/fanotify/fanotify_user.c
b/fs/notify/fanotify/fanotify_user.c
index caa1211bac8c..cf3584351e00 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -283,19 +283,44 @@ static int create_fd(struct fsnotify_group *group, const struct
path *path,
return client_fd;
}
+static int process_access_response_info(int fd, const char __user *info, size_t
info_len,
+ struct fanotify_response_info_audit_rule *friar)
I prefer to keep lines within 80 columns, unless there is really good
reason (like with strings) to have them longer.
BTW, why do you call the info structure 'friar'? I feel some language twist
escapes me ;)
+{
+ if (fd == FAN_NOFD)
+ return -ENOENT;
I would not test 'fd' in this function at all. After all it is not part of
the response info structure and you do check it in
process_access_response() anyway.
+
+ if (info_len != sizeof(*friar))
+ return -EINVAL;
+
+ if (copy_from_user(friar, info, sizeof(*friar)))
+ return -EFAULT;
+
+ if (friar->hdr.type != FAN_RESPONSE_INFO_AUDIT_RULE)
+ return -EINVAL;
+ if (friar->hdr.pad != 0)
+ return -EINVAL;
+ if (friar->hdr.len != sizeof(*friar))
+ return -EINVAL;
+
+ return info_len;
+}
+
...
@@ -327,10 +359,18 @@ static int process_access_response(struct
fsnotify_group *group,
return -EINVAL;
}
- if (fd < 0)
+ if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
- if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
+ if (response & FAN_INFO) {
+ ret = process_access_response_info(fd, info, info_len, &friar);
+ if (ret < 0)
+ return ret;
+ } else {
+ ret = 0;
+ }
+
+ if (fd < 0)
return -EINVAL;
And here I'd do:
if (fd == FAN_NOFD)
return 0;
if (fd < 0)
return -EINVAL;
As we talked in previous revisions we'd specialcase FAN_NOFD to just verify
extra info is understood by the kernel so that application writing fanotify
responses has a way to check which information it can provide to the
kernel.
Honza
--
Jan Kara <jack(a)suse.com>
SUSE Labs, CR