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

From: Amir Goldstein
Date: Wed Aug 10 2022 - 02:23:08 EST

Next message: Huang, Ying: "Re: [PATCH v3 4/7] mm/thp: Carry over dirty bit when thp splits on pmd"
Previous message: Muchun Song: "Re: [PATCH] hugetlb: freeze allocated pages before creating hugetlb pages"
In reply to: Richard Guy Briggs: "[PATCH v4 2/4] fanotify: define struct members to hold response decision context"
Next in thread: kernel test robot: "Re: [PATCH v4 2/4] fanotify: define struct members to hold response decision context"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

[+linux-api]

On Tue, Aug 9, 2022 at 7:23 PM Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Link: https://lore.kernel.org/r/20201001101219.GE17860@xxxxxxxxxxxxxx
> Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx>
> ---

Looks mostly fine.
A few small bugs and style suggestions
and one UAPI improvement suggestion.

> fs/notify/fanotify/fanotify.c | 10 ++-
> fs/notify/fanotify/fanotify.h | 2 +
> fs/notify/fanotify/fanotify_user.c | 104 +++++++++++++++++++++++------
> include/linux/fanotify.h | 5 ++
> include/uapi/linux/fanotify.h | 27 +++++++-
> 5 files changed, 123 insertions(+), 25 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 4f897e109547..0f36062521f4 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -262,13 +262,16 @@ static int fanotify_get_response(struct fsnotify_group *group,
> }
>
> /* userspace responded, convert to something usable */
> - switch (event->response & ~FAN_AUDIT) {
> + switch (event->response & FANOTIFY_RESPONSE_ACCESS) {
> case FAN_ALLOW:
> ret = 0;
> break;
> case FAN_DENY:
> - default:
> ret = -EPERM;
> + break;
> + default:
> + ret = -EINVAL;
> + break;

This is very odd.
Why has this changed?
The return value here is going to the process that
is trying to access the file.

> }
>
> /* Check if the response should be audited */
> @@ -560,6 +563,8 @@ static struct fanotify_event *fanotify_alloc_perm_event(const struct path *path,
>
> pevent->fae.type = FANOTIFY_EVENT_TYPE_PATH_PERM;
> pevent->response = 0;
> + pevent->info_len = 0;
> + pevent->info_buf = NULL;
> pevent->state = FAN_EVENT_INIT;
> pevent->path = *path;
> path_get(path);
> @@ -996,6 +1001,7 @@ static void fanotify_free_path_event(struct fanotify_event *event)
> static void fanotify_free_perm_event(struct fanotify_event *event)
> {
> path_put(fanotify_event_path(event));
> + kfree(FANOTIFY_PERM(event)->info_buf);
> kmem_cache_free(fanotify_perm_event_cachep, FANOTIFY_PERM(event));
> }
>
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index abfa3712c185..14c30e173632 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -428,6 +428,8 @@ struct fanotify_perm_event {
> u32 response; /* userspace answer to the event */
> unsigned short state; /* state of the event */
> int fd; /* fd we passed to userspace for this event */
> + size_t info_len;
> + char *info_buf;
> };
>
> static inline struct fanotify_perm_event *
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index ff67ca0d25cc..a4ae953f0e62 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -289,13 +289,18 @@ 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,
> + size_t info_len, char *info_buf)
> __releases(&group->notification_lock)
> {
> bool destroy = false;
>
> assert_spin_locked(&group->notification_lock);
> - event->response = response;
> + event->response = response->response & ~FAN_INFO;
> + if (response->response & FAN_INFO) {
> + event->info_len = info_len;
> + event->info_buf = info_buf;
> + }
> if (event->state == FAN_EVENT_CANCELED)
> destroy = true;
> else
> @@ -306,33 +311,71 @@ static void finish_permission_event(struct fsnotify_group *group,
> }
>
> 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@xxxxxxxxxxxxxx/

> + if (response & FAN_INFO) {

Please split this out to helper process_response_info() and
optionally also helper process_response_info_audit_rule()

> + size_t c = count;
> + const char __user *ib = buf;
>
> + if (c <= 0)
> + return -EINVAL;

This was already checked by the caller.
If you think we need this defence use if (WARN_ON_ONCE())

> + while (c >= sizeof(info_hdr)) {

This while() is a bit confusing.
It suggests that the parser may process multiple info records,
but the code below uses 'count' and assumed single audit rule
record.

Maybe just change this to:
if (WARN_ON_ONCE(c < sizeof(info_hdr))
return -EINVAL

Until the code can really handle multiple records.

> + if (copy_from_user(&info_hdr, ib, sizeof(info_hdr)))
> + return -EFAULT;
> + if (info_hdr.pad != 0)
> + return -EINVAL;
> + if (c < info_hdr.len)
> + return -EINVAL;
> + switch (info_hdr.type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + break;
> + case FAN_RESPONSE_INFO_NONE:
> + default:
> + return -EINVAL;
> + }
> + c -= info_hdr.len;
> + ib += info_hdr.len;
> + }
> + if (c != 0)
> + return -EINVAL;
> + /* Simplistic check for now */
> + if (count != sizeof(struct fanotify_response_info_audit_rule))
> + return -EINVAL;
> + info_buf = kmalloc(sizeof(struct fanotify_response_info_audit_rule),
> + GFP_KERNEL);
> + if (!info_buf)
> + return -ENOMEM;
> + if (copy_from_user(info_buf, buf, count))
> + return -EFAULT;

info_buf allocation is leaked here and also in case 'fd' is not found.

> + }
> spin_lock(&group->notification_lock);
> list_for_each_entry(event, &group->fanotify_data.access_list,
> fae.fse.list) {
> @@ -340,7 +383,9 @@ static int process_access_response(struct fsnotify_group *group,
> continue;
>
> list_del_init(&event->fae.fse.list);
> - finish_permission_event(group, event, response);
> + /* finish_permission_event() eats info_buf */
> + finish_permission_event(group, event, response_struct,
> + count, info_buf);
> wake_up(&group->fanotify_data.access_waitq);
> return 0;
> }
> @@ -802,9 +847,14 @@ static ssize_t fanotify_read(struct file *file, char __user *buf,
> fsnotify_destroy_event(group, &event->fse);
> } else {
> if (ret <= 0) {
> + struct fanotify_response response = {
> + .fd = FAN_NOFD,
> + .response = FAN_DENY };
> +
> spin_lock(&group->notification_lock);
> finish_permission_event(group,
> - FANOTIFY_PERM(event), FAN_DENY);
> + FANOTIFY_PERM(event), &response,
> + 0, NULL);
> wake_up(&group->fanotify_data.access_waitq);
> } else {
> spin_lock(&group->notification_lock);
> @@ -827,26 +877,33 @@ 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;
> + const char __user *info_buf = buf + sizeof(struct fanotify_response);
> + size_t c;
>
> if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS))
> return -EINVAL;
>
> group = file->private_data;
>
> - 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?

> + } else {
> + if (c != 0)
> + return -EINVAL;
> + }
> + ret = process_access_response(group, &response, info_buf, c);
> if (ret < 0)
> count = ret;
>
> @@ -857,6 +914,9 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> {
> struct fsnotify_group *group = file->private_data;
> struct fsnotify_event *fsn_event;
> + struct fanotify_response response = {
> + .fd = FAN_NOFD,
> + .response = FAN_ALLOW };
>
> /*
> * Stop new events from arriving in the notification queue. since
> @@ -876,7 +936,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> event = list_first_entry(&group->fanotify_data.access_list,
> struct fanotify_perm_event, fae.fse.list);
> list_del_init(&event->fae.fse.list);
> - finish_permission_event(group, event, FAN_ALLOW);
> + finish_permission_event(group, event, &response, 0, NULL);
> spin_lock(&group->notification_lock);
> }
>
> @@ -893,7 +953,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> fsnotify_destroy_event(group, fsn_event);
> } else {
> finish_permission_event(group, FANOTIFY_PERM(event),
> - FAN_ALLOW);
> + &response, 0, NULL);
> }
> spin_lock(&group->notification_lock);
> }
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index edc28555814c..ce9f97eb69f2 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -114,6 +114,11 @@
> #define ALL_FANOTIFY_EVENT_BITS (FANOTIFY_OUTGOING_EVENTS | \
> FANOTIFY_EVENT_FLAGS)
>
> +/* This mask is to check for invalid bits of a user space permission response */
> +#define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY)
> +#define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO)
> +#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS)
> +
> /* Do not use these old uapi constants internally */
> #undef FAN_ALL_CLASS_BITS
> #undef FAN_ALL_INIT_FLAGS
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index f1f89132d60e..4d08823a5698 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -180,15 +180,40 @@ 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 information 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_NONE 0
> +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> +
> struct fanotify_response {
> __s32 fd;
> __u32 response;
> };
>
> +struct fanotify_response_info_header {
> + __u8 type;
> + __u8 pad;
> + __u16 len;
> +};
> +
> +struct fanotify_response_info_audit_rule {
> + struct fanotify_response_info_header hdr;
> + __u32 audit_rule;
> +};
> +
> /* Legit userspace responses to a _PERM event */
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> -#define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
> +#define FAN_AUDIT 0x10 /* Bitmask to create audit record for result */
> +#define FAN_INFO 0x20 /* Bitmask to indicate additional information */
>
> /* No fd set in event */
> #define FAN_NOFD -1
> --
> 2.27.0
>

Next message: Huang, Ying: "Re: [PATCH v3 4/7] mm/thp: Carry over dirty bit when thp splits on pmd"
Previous message: Muchun Song: "Re: [PATCH] hugetlb: freeze allocated pages before creating hugetlb pages"
In reply to: Richard Guy Briggs: "[PATCH v4 2/4] fanotify: define struct members to hold response decision context"
Next in thread: kernel test robot: "Re: [PATCH v4 2/4] fanotify: define struct members to hold response decision context"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]