lkml.org 
[lkml]   [2022]   [May]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [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@redhat.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@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>
    > 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.

    > 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

    \
     
     \ /
      Last update: 2022-05-03 02:18    [W:3.896 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site