lkml.org 
[lkml]   [2022]   [Dec]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@redhat.com>
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara <jack@suse.cz>
> Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> Signed-off-by: Richard Guy Briggs <rgb@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@suse.com>
SUSE Labs, CR

\
 
 \ /
  Last update: 2022-12-16 17:45    [W:0.140 / U:0.044 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site