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

[+linux-api] On Tue, Aug 9, 2022 at 7:23 PM Richard Guy Briggs <rgb(a)redhat.com&gt; 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, an audit 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&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; > ---

> static int process_access_response(struct fsnotify_group *group, > - struct fanotify_response *response_struct) > + struct fanotify_response *response_struct, > + const char __user *buf, > + size_t count) > { > struct fanotify_perm_event *event; > int fd = response_struct->fd; > u32 response = response_struct->response; > + struct fanotify_response_info_header info_hdr; > + char *info_buf = NULL; > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group, > - fd, response); > + pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%lu\n", __func__, > + group, fd, response, info_buf, count); > /* > * make sure the response is valid, if invalid we do nothing and either > * userspace can send a valid response or we will clean it up after the > * timeout > */ > - switch (response & ~FAN_AUDIT) { > + if (response & ~FANOTIFY_RESPONSE_VALID_MASK) > + return -EINVAL; > + switch (response & FANOTIFY_RESPONSE_ACCESS) { > case FAN_ALLOW: > case FAN_DENY: > break; > default: > return -EINVAL; > } > - > - if (fd < 0) > - return -EINVAL; > - > if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT)) > return -EINVAL; > + if (fd < 0) > + return -EINVAL; Since you did not accept my suggestion of FAN_TEST [1], I am not sure why this check was moved. However, if you move this check past FAN_INFO processing, you could change the error value to -ENOENT, same as the return value for an fd that is >= 0 but does not correspond to any pending permission event. The idea was that userspace could write a test fanotify_response_info_audit_rule payload to fanotify fd with FAN_NOFD in the response.fd field. On old kernel, this will return EINVAL. On new kernel, if the fanotify_response_info_audit_rule payload passes all the validations, this will do nothing and return ENOENT. [1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxi+8HUqyGxQBNMqSong92nreOWLKd...

> - if (count < sizeof(response)) > - return -EINVAL; > - > - count = sizeof(response); > - > pr_debug("%s: group=%p count=%zu\n", __func__, group, count); > > - if (copy_from_user(&response, buf, count)) > + if (count < sizeof(response)) > + return -EINVAL; > + if (copy_from_user(&response, buf, sizeof(response))) > return -EFAULT; > > - ret = process_access_response(group, &response); > + c = count - sizeof(response); > + if (response.response & FAN_INFO) { > + if (c < sizeof(struct fanotify_response_info_header)) > + return -EINVAL; Should FAN_INFO require FAN_AUDIT?