Date:   Tue, 3 May 2022 21:33:35 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, Eric Paris <eparis@...isplace.org>,
        Steve Grubb <sgrubb@...hat.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v2 2/3] fanotify: define struct members to hold response
 decision context

On 2022-05-02 20:16, Paul Moore wrote:
> On Thu, Apr 28, 2022 at 8:45 PM Richard Guy Briggs <rgb@...hat.com> 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@...hat.com>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <jack@...e.cz>
> > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>
> > Link: https://lore.kernel.org/r/17660b3f2817e5c0a19d1e9e5d40b53ff4561845.1651174324.git.rgb@redhat.com
> > ---
> >  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.

It was a harmless part of the original patch, but I agree it should go.

> >         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));

I've been wrestling with the possibility of doing a split between what
is presented to userspace and what's used in the kernel for struct
fanotify_response, while attempting to future-proof it.

At the moment, since the extra_info_buf is either zero or has a fixed
size for the "RULE" type, it seemed to be most efficient to do a static
allocation on the stack upon entry into fanotify_write() that was
only 4 octets more than the type "NONE" case.  Later, if a new type has
a variable extra_info_buf size, it can be internally allocated
dynamically.

> > +       }
> >         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.

No.  I shrank it at Jan's suggestion.  I think I agree with you that
the response field should be kept at u32 as it is defined in userspace
and purge the doubt about what would happen with a new userspace with
an old kernel.

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

This does make sense and my only concern would be a variable-length
type.  There isn't any reason to hide it.  If userspace chooses to use
the old interface and omit the type field then it defaults to NONE.

If future types with variable data are defined, the first field could be
a u32 that unions with the rule number that won't change the struct
size.

Thanks for the feedback.

> paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635

Powered by blists - more mailing lists