Re: [PATCH v5 2/3] fanotify: define struct members to hold response decision context

[Jan Kara <jack@suse.cz>]

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