2022-04-29 09:45:39

by Richard Guy Briggs

[permalink] [raw]
Subject: [PATCH v2 1/3] fanotify: Ensure consistent variable type for response

The user space API for the response variable is __u32. This patch makes
sure that the whole path through the kernel uses __u32 so that there is
no sign extension or truncation of the user space response.

Suggested-by: Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
Signed-off-by: Richard Guy Briggs <[email protected]>
Link: https://lore.kernel.org/r/aa98a3ad00666a6fc0ce411755de4a1a60f5c0cd.1651174324.git.rgb@redhat.com
---
fs/notify/fanotify/fanotify.h | 2 +-
fs/notify/fanotify/fanotify_user.c | 6 +++---
2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index a3d5b751cac5..70acfd497771 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -425,7 +425,7 @@ FANOTIFY_PE(struct fanotify_event *event)
struct fanotify_perm_event {
struct fanotify_event fae;
struct path path;
- unsigned short response; /* userspace answer to the 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 */
};
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 9b32b76a9c30..694516470660 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -289,7 +289,7 @@ 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,
- unsigned int response)
+ __u32 response)
__releases(&group->notification_lock)
{
bool destroy = false;
@@ -310,9 +310,9 @@ static int process_access_response(struct fsnotify_group *group,
{
struct fanotify_perm_event *event;
int fd = response_struct->fd;
- int response = response_struct->response;
+ __u32 response = response_struct->response;

- pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
+ pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
fd, response);
/*
* make sure the response is valid, if invalid we do nothing and either
--
2.27.0


2022-05-03 01:28:55

by Paul Moore

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] fanotify: Ensure consistent variable type for response

On Thu, Apr 28, 2022 at 8:45 PM Richard Guy Briggs <[email protected]> wrote:
>
> The user space API for the response variable is __u32. This patch makes
> sure that the whole path through the kernel uses __u32 so that there is
> no sign extension or truncation of the user space response.
>
> Suggested-by: Steve Grubb <[email protected]>
> Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
> Signed-off-by: Richard Guy Briggs <[email protected]>
> Link: https://lore.kernel.org/r/aa98a3ad00666a6fc0ce411755de4a1a60f5c0cd.1651174324.git.rgb@redhat.com
> ---
> fs/notify/fanotify/fanotify.h | 2 +-
> fs/notify/fanotify/fanotify_user.c | 6 +++---
> 2 files changed, 4 insertions(+), 4 deletions(-)

It seems like audit_fanotify()/__audit_fanotify() should also be
changed, yes? Granted, in this case it's an unsigned int to u32
conversion so not really all that critical, but if you are going to
update the fanotify code you might as well update the audit code as
well for the sake of completeness.

--
paul-moore.com

2022-05-04 12:25:58

by Richard Guy Briggs

[permalink] [raw]
Subject: Re: [PATCH v2 1/3] fanotify: Ensure consistent variable type for response

On 2022-05-02 20:16, Paul Moore wrote:
> On Thu, Apr 28, 2022 at 8:45 PM Richard Guy Briggs <[email protected]> wrote:
> >
> > The user space API for the response variable is __u32. This patch makes
> > sure that the whole path through the kernel uses __u32 so that there is
> > no sign extension or truncation of the user space response.
> >
> > Suggested-by: Steve Grubb <[email protected]>
> > Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > Link: https://lore.kernel.org/r/aa98a3ad00666a6fc0ce411755de4a1a60f5c0cd.1651174324.git.rgb@redhat.com
> > ---
> > fs/notify/fanotify/fanotify.h | 2 +-
> > fs/notify/fanotify/fanotify_user.c | 6 +++---
> > 2 files changed, 4 insertions(+), 4 deletions(-)
>
> It seems like audit_fanotify()/__audit_fanotify() should also be
> changed, yes? Granted, in this case it's an unsigned int to u32
> conversion so not really all that critical, but if you are going to
> update the fanotify code you might as well update the audit code as
> well for the sake of completeness.

Yes, that was somewhere in the back of my mind but forgot to come back
to it. Thanks for catching that.

> paul-moore.com

- RGB

--
Richard Guy Briggs <[email protected]>
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