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

[Jan Kara <jack@suse.cz>]

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 <rgb@redhat.com> 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@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>
> > ---

...

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

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.

> > -       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 <jack@suse.com>
SUSE Labs, CR