| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHC9VhQ3Qtpwhj6TeMR7rmdbUe_6VRHU9OymmDoDdsazeGuNKA@mail.gmail.com>
Date: Mon, 2 May 2022 20:16:13 -0400
From: Paul Moore <paul@...l-moore.com>
To: Richard Guy Briggs <rgb@...hat.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 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.
> 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
Powered by blists - more mailing lists