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

On Thu, Apr 28, 2022 at 8:45 PM Richard Guy Briggs <rgb(a)redhat.com&gt; wrote: > This patch adds 2 structure members to the response returned from user > space on a permission event. The first field is 16 bits for the context > type. The context type will describe what the meaning is of the second > field. The default is none. The patch defines one additional context > type which means that the second field is a 32-bit rule number. This > will allow for the creation of other context types in the future if > other users of the API identify different needs. The second field size > is defined by the context type and can be used to pass along the data > described by the context. > > To support this, there is a macro for user space to check that the data > being sent is valid. Of course, without this check, anything that > overflows the bit field will trigger an EINVAL based on the use of > FAN_INVALID_RESPONSE_MASK in process_access_response(). > > Suggested-by: Steve Grubb <sgrubb(a)redhat.com&gt; > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 > Suggested-by: Jan Kara <jack(a)suse.cz&gt; > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz > Signed-off-by: Richard Guy Briggs <rgb(a)redhat.com&gt; > Link: https://lore.kernel.org/r/17660b3f2817e5c0a19d1e9e5d40b53ff4561845.165117... > --- > fs/notify/fanotify/fanotify.c | 1 - > fs/notify/fanotify/fanotify.h | 4 +- > fs/notify/fanotify/fanotify_user.c | 59 ++++++++++++++++++++---------- > include/linux/fanotify.h | 3 ++ > include/uapi/linux/fanotify.h | 27 +++++++++++++- > 5 files changed, 72 insertions(+), 22 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 985e995d2a39..00aff6e29bf8 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -266,7 +266,6 @@ static int fanotify_get_response(struct fsnotify_group *group, > case FAN_ALLOW: > ret = 0; > break; > - case FAN_DENY: I personally would drop this from the patch if it was me, it doesn't change the behavior so it falls under the "noise" category, which could be a problem considering the lack of response on the original posting and this one. Small, focused patches have a better shot of review/merging.

> default: > ret = -EPERM; > } ... > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 694516470660..f1ff4cf683fb 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -289,13 +289,19 @@ static int create_fd(struct fsnotify_group *group, struct path *path, > */ > static void finish_permission_event(struct fsnotify_group *group, > struct fanotify_perm_event *event, > - __u32 response) > + struct fanotify_response *response) > __releases(&group->notification_lock) > { > bool destroy = false; > > assert_spin_locked(&group->notification_lock); > - event->response = response; > + event->response = response->response; > + event->extra_info_type = response->extra_info_type; > + switch (event->extra_info_type) { > + case FAN_RESPONSE_INFO_AUDIT_RULE: > + memcpy(event->extra_info_buf, response->extra_info_buf, > + sizeof(struct fanotify_response_audit_rule)); Since the fanotify_perm_event:extra_info_buf and fanotify_response:extra_info_buf are the same type/length, and they will be the same regardless of the extra_info_type field, why not simply get rid of the above switch statement and do something like this: memcpy(event->extra_info_buf, response->extra_info_buf, sizeof(response->extra_info_buf));

> + } > if (event->state == FAN_EVENT_CANCELED) > destroy = true; > else ... > @@ -827,26 +845,25 @@ static ssize_t fanotify_read(struct file *file, char __user *buf, > > static ssize_t fanotify_write(struct file *file, const char __user *buf, size_t count, loff_t *pos) > { > - struct fanotify_response response = { .fd = -1, .response = -1 }; > + struct fanotify_response response; > struct fsnotify_group *group; > int ret; > + size_t size = min(count, sizeof(struct fanotify_response)); > > if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) > return -EINVAL; > > group = file->private_data; > > - if (count < sizeof(response)) > + if (count < offsetof(struct fanotify_response, extra_info_buf)) > return -EINVAL; Is this why you decided to shrink the fanotify_response:response field from 32-bits to 16-bits? I hope not. I would suggest both keeping the existing response field as 32-bits and explicitly checking for writes that are either the existing/compat length as well as the newer, longer length.

> - count = sizeof(response); > - > pr_debug("%s: group=%p count=%zu\n", __func__, group, count); > > - if (copy_from_user(&response, buf, count)) > + if (copy_from_user(&response, buf, size)) > return -EFAULT; > > - ret = process_access_response(group, &response); > + ret = process_access_response(group, &response, count); > if (ret < 0) > count = ret; > ... > diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h > index e8ac38cc2fd6..efb5a3a6f814 100644 > --- a/include/uapi/linux/fanotify.h > +++ b/include/uapi/linux/fanotify.h > @@ -179,9 +179,34 @@ struct fanotify_event_info_error { > __u32 error_count; > }; > > +/* > + * User space may need to record additional information about its decision. > + * The extra information type records what kind of information is included. > + * The default is none. We also define an extra informaion buffer whose > + * size is determined by the extra information type. > + * > + * If the context type is Rule, then the context following is the rule number > + * that triggered the user space decision. > + */ > + > +#define FAN_RESPONSE_INFO_AUDIT_NONE 0 > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1 > + > +struct fanotify_response_audit_rule { > + __u32 rule; > +}; > + > +#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX \ > + (sizeof(union { \ > + struct fanotify_response_audit_rule r; \ > + /* add other extra info structures here */ \ > + })) > + > struct fanotify_response { > __s32 fd; > - __u32 response; > + __u16 response; > + __u16 extra_info_type; > + char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX]; > }; Since both the kernel and userspace are going to need to agree on the content and formatting of the fanotify_response:extra_info_buf field, why is it hidden behind a char array? You might as well get rid of that abstraction and put the union directly in the fanotify_response struct. It is possible you could also get rid of the fanotify_response_audit_rule struct this way too and just access the rule scalar directly.

paul-moore.com