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

On Wed 10-08-22 08:22:49, Amir Goldstein wrote:

[+linux-api]

On Tue, Aug 9, 2022 at 7:23 PM Richard Guy Briggs [off-list ref] wrote:

quoted

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 <redacted>
Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 (local)
Suggested-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz (local)
Signed-off-by: Richard Guy Briggs <redacted>
---

...

quoted

 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+8HUqyGxQBNMqSong92nreOWLKdy9MCrYg8wgW9Dj4g@mail.gmail.com/ (local)

Yes. Richard, if you don't like the FAN_TEST proposal from Amir, please
explain (preferably also with sample code) how you imagine userspace will
decide whether to use FAN_INFO flag in responses or not. Because if it will
just blindly set it, that will result in all permission events to finished
with EPERM for kernels not recognizing FAN_INFO.

quoted

-       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?

Currently we could but longer term not all additional info needs to be
related to audit so probably I'd not require that even now (which results
in info being effectively ignored after it is parsed).

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR