* [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-16 20:22 ` Richard Guy Briggs
0 siblings, 0 replies; 33+ messages in thread
From: Richard Guy Briggs @ 2022-05-16 20:22 UTC (permalink / raw)
To: Linux-Audit Mailing List, LKML, linux-fsdevel
Cc: Paul Moore, Eric Paris, Steve Grubb, Richard Guy Briggs,
Jan Kara, Amir Goldstein
This patch adds 2 structure members to the response returned from user
space on a permission event. The first field is 32 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 union containing 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>
---
fs/notify/fanotify/fanotify.c | 2 +-
fs/notify/fanotify/fanotify.h | 2 +
fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
include/linux/fanotify.h | 3 ++
include/uapi/linux/fanotify.h | 22 ++++++++-
5 files changed, 75 insertions(+), 28 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 985e995d2a39..ea0e60488f12 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
}
/* userspace responded, convert to something usable */
- switch (event->response & ~FAN_AUDIT) {
+ switch (event->response & ~(FAN_AUDIT | FAN_EXTRA)) {
case FAN_ALLOW:
ret = 0;
break;
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index d66668e06bee..eb7ec1f2a26e 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -426,8 +426,10 @@ struct fanotify_perm_event {
struct fanotify_event fae;
struct path path;
u32 response; /* userspace answer to the event */
+ u32 extra_info_type;
unsigned short state; /* state of the event */
int fd; /* fd we passed to userspace for this event */
+ union fanotify_response_extra extra_info;
};
static inline struct fanotify_perm_event *
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 721e777ea90b..1c4067e29f2e 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -289,13 +289,22 @@ 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 & ~FAN_EXTRA;
+ if (response->response & FAN_EXTRA) {
+ event->extra_info_type = response->extra_info_type;
+ switch (event->extra_info_type) {
+ case FAN_RESPONSE_INFO_AUDIT_RULE:
+ event->extra_info.audit_rule = response->extra_info.audit_rule;
+ }
+ } else {
+ event->extra_info_type = FAN_RESPONSE_INFO_NONE;
+ }
if (event->state == FAN_EVENT_CANCELED)
destroy = true;
else
@@ -306,33 +315,40 @@ 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,
+ size_t count)
{
struct fanotify_perm_event *event;
int fd = response_struct->fd;
u32 response = response_struct->response;
- pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
- fd, response);
+ pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
+ group, fd, response, response_struct->extra_info_type, count);
+ if (fd < 0)
+ return -EINVAL;
/*
* 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) {
- case FAN_ALLOW:
- case FAN_DENY:
- break;
- default:
- return -EINVAL;
- }
-
- if (fd < 0)
+ if (FAN_INVALID_RESPONSE_MASK(response))
return -EINVAL;
-
if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
-
+ if (response & FAN_EXTRA) {
+ if (count < offsetofend(struct fanotify_response, extra_info_type))
+ return -EINVAL;
+ switch (response_struct->extra_info_type) {
+ case FAN_RESPONSE_INFO_NONE:
+ break;
+ case FAN_RESPONSE_INFO_AUDIT_RULE:
+ if (count < offsetofend(struct fanotify_response, extra_info))
+ return -EINVAL;
+ break;
+ default:
+ return -EINVAL;
+ }
+ }
spin_lock(&group->notification_lock);
list_for_each_entry(event, &group->fanotify_data.access_list,
fae.fse.list) {
@@ -340,7 +356,7 @@ 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(group, event, response_struct);
wake_up(&group->fanotify_data.access_waitq);
return 0;
}
@@ -802,9 +818,13 @@ 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);
wake_up(&group->fanotify_data.access_waitq);
} else {
spin_lock(&group->notification_lock);
@@ -827,26 +847,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 < offsetofend(struct fanotify_response, 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 (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;
@@ -857,6 +876,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 +898,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);
spin_lock(&group->notification_lock);
}
@@ -893,7 +915,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);
}
spin_lock(&group->notification_lock);
}
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index 419cadcd7ff5..63a8494e782e 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -113,6 +113,9 @@
#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 FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
+
/* 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 e8ac38cc2fd6..a94f4143601f 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -179,15 +179,35 @@ 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_NONE 0
+#define FAN_RESPONSE_INFO_AUDIT_RULE 1
+
+union fanotify_response_extra {
+ __u32 audit_rule;
+};
+
struct fanotify_response {
__s32 fd;
__u32 response;
+ __u32 extra_info_type;
+ union fanotify_response_extra extra_info;
};
/* 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_EXTRA 0x20 /* Bitmask to indicate additional information */
/* No fd set in event */
#define FAN_NOFD -1
--
2.27.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-16 20:22 ` Richard Guy Briggs
@ 2022-05-17 5:37 ` Amir Goldstein
-1 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-17 5:37 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, Paul Moore,
Eric Paris, Steve Grubb, Jan Kara
On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
> ---
> fs/notify/fanotify/fanotify.c | 2 +-
> fs/notify/fanotify/fanotify.h | 2 +
> fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
> include/linux/fanotify.h | 3 ++
> include/uapi/linux/fanotify.h | 22 ++++++++-
> 5 files changed, 75 insertions(+), 28 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 985e995d2a39..ea0e60488f12 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
> }
>
> /* userspace responded, convert to something usable */
> - switch (event->response & ~FAN_AUDIT) {
> + switch (event->response & ~(FAN_AUDIT | FAN_EXTRA)) {
> case FAN_ALLOW:
> ret = 0;
> break;
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index d66668e06bee..eb7ec1f2a26e 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> struct fanotify_event fae;
> struct path path;
> u32 response; /* userspace answer to the event */
> + u32 extra_info_type;
> unsigned short state; /* state of the event */
> int fd; /* fd we passed to userspace for this event */
> + union fanotify_response_extra extra_info;
> };
>
> static inline struct fanotify_perm_event *
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 721e777ea90b..1c4067e29f2e 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -289,13 +289,22 @@ 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 & ~FAN_EXTRA;
> + if (response->response & FAN_EXTRA) {
> + event->extra_info_type = response->extra_info_type;
> + switch (event->extra_info_type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + event->extra_info.audit_rule = response->extra_info.audit_rule;
> + }
> + } else {
> + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> + }
> if (event->state == FAN_EVENT_CANCELED)
> destroy = true;
> else
> @@ -306,33 +315,40 @@ 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,
> + size_t count)
> {
> struct fanotify_perm_event *event;
> int fd = response_struct->fd;
> u32 response = response_struct->response;
>
> - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> - fd, response);
> + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> + group, fd, response, response_struct->extra_info_type, count);
> + if (fd < 0)
> + return -EINVAL;
> /*
> * 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) {
> - case FAN_ALLOW:
> - case FAN_DENY:
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - if (fd < 0)
> + if (FAN_INVALID_RESPONSE_MASK(response))
That is a logic change, because now the response value of 0 becomes valid.
Since you did not document this change in the commit message I assume this was
non intentional?
However, this behavior change is something that I did ask for, but it should be
done is a separate commit:
/* These are NOT bitwise flags. Both bits can be used together. */
#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
#define FANOTIFY_RESPONSE_ACCESS \
(FAN_TEST|FAN_ALLOW | FAN_DENY)
...
int access = response & FANOTIFY_RESPONSE_ACCESS;
1. Do return EINVAL for access == 0
2. Let all the rest of the EINVAL checks run (including extra type)
3. Move if (fd < 0) to last check
4. Add if (!access) return 0 before if (fd < 0)
That will provide a mechanism for userspace to probe the
kernel support for extra types in general and specific types
that it may respond with.
> return -EINVAL;
> -
> if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> return -EINVAL;
> -
> + if (response & FAN_EXTRA) {
> + if (count < offsetofend(struct fanotify_response, extra_info_type))
> + return -EINVAL;
> + switch (response_struct->extra_info_type) {
> + case FAN_RESPONSE_INFO_NONE:
> + break;
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + if (count < offsetofend(struct fanotify_response, extra_info))
That's a trap right there.
In future kernel, if someone adds a 64bit member to the extra_info union
existing binaries will start failing.
Also since struct fanotify_response is not packed, a 64bit member in the
union will change the alignment of extra_info union.
The use of a union in UAPI seems to be asking for trouble.
You should probably follow the pattern of fanotify_event_info_* structs.
It's more work, but I don't see another way.
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> spin_lock(&group->notification_lock);
> list_for_each_entry(event, &group->fanotify_data.access_list,
> fae.fse.list) {
> @@ -340,7 +356,7 @@ 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(group, event, response_struct);
> wake_up(&group->fanotify_data.access_waitq);
> return 0;
> }
> @@ -802,9 +818,13 @@ 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);
> wake_up(&group->fanotify_data.access_waitq);
> } else {
> spin_lock(&group->notification_lock);
> @@ -827,26 +847,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 < offsetofend(struct fanotify_response, 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 (copy_from_user(&response, buf, size))
> return -EFAULT;
>
> - ret = process_access_response(group, &response);
> + ret = process_access_response(group, &response, count);
We did not copy count bytes of response. We copied size bytes.
> if (ret < 0)
> count = ret;
>
> @@ -857,6 +876,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 +898,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);
> spin_lock(&group->notification_lock);
> }
>
> @@ -893,7 +915,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);
> }
> spin_lock(&group->notification_lock);
> }
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 419cadcd7ff5..63a8494e782e 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -113,6 +113,9 @@
> #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 FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
> +
Please drop this macro and follow the pattern of FANOTIFY_{INIT,MARK,EVENT}_*
#define FANOTIFY_RESPONSE_ACCESS \
(FAN_ALLOW | FAN_DENY)
#define FANOTIFY_RESPONSE_FLAGS \
(FAN_AUDIT | FAN_EXTRA)
#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 e8ac38cc2fd6..a94f4143601f 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -179,15 +179,35 @@ 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
typo: informaion
> + * 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
> +
> +union fanotify_response_extra {
> + __u32 audit_rule;
> +};
> +
> struct fanotify_response {
> __s32 fd;
> __u32 response;
> + __u32 extra_info_type;
> + union fanotify_response_extra extra_info;
IIRC, Jan wanted this to be a variable size record with info_type and info_len.
I don't know if we want to make this flexible enough to allow for multiple
records in the future like we do in events, but the common wisdom of
the universe says that if we don't do it, we will need it.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-17 5:37 ` Amir Goldstein
0 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-17 5:37 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Jan Kara, LKML, Linux-Audit Mailing List, linux-fsdevel, Eric Paris
On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
> ---
> fs/notify/fanotify/fanotify.c | 2 +-
> fs/notify/fanotify/fanotify.h | 2 +
> fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
> include/linux/fanotify.h | 3 ++
> include/uapi/linux/fanotify.h | 22 ++++++++-
> 5 files changed, 75 insertions(+), 28 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 985e995d2a39..ea0e60488f12 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
> }
>
> /* userspace responded, convert to something usable */
> - switch (event->response & ~FAN_AUDIT) {
> + switch (event->response & ~(FAN_AUDIT | FAN_EXTRA)) {
> case FAN_ALLOW:
> ret = 0;
> break;
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index d66668e06bee..eb7ec1f2a26e 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> struct fanotify_event fae;
> struct path path;
> u32 response; /* userspace answer to the event */
> + u32 extra_info_type;
> unsigned short state; /* state of the event */
> int fd; /* fd we passed to userspace for this event */
> + union fanotify_response_extra extra_info;
> };
>
> static inline struct fanotify_perm_event *
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 721e777ea90b..1c4067e29f2e 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -289,13 +289,22 @@ 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 & ~FAN_EXTRA;
> + if (response->response & FAN_EXTRA) {
> + event->extra_info_type = response->extra_info_type;
> + switch (event->extra_info_type) {
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + event->extra_info.audit_rule = response->extra_info.audit_rule;
> + }
> + } else {
> + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> + }
> if (event->state == FAN_EVENT_CANCELED)
> destroy = true;
> else
> @@ -306,33 +315,40 @@ 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,
> + size_t count)
> {
> struct fanotify_perm_event *event;
> int fd = response_struct->fd;
> u32 response = response_struct->response;
>
> - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> - fd, response);
> + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> + group, fd, response, response_struct->extra_info_type, count);
> + if (fd < 0)
> + return -EINVAL;
> /*
> * 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) {
> - case FAN_ALLOW:
> - case FAN_DENY:
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - if (fd < 0)
> + if (FAN_INVALID_RESPONSE_MASK(response))
That is a logic change, because now the response value of 0 becomes valid.
Since you did not document this change in the commit message I assume this was
non intentional?
However, this behavior change is something that I did ask for, but it should be
done is a separate commit:
/* These are NOT bitwise flags. Both bits can be used together. */
#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
#define FANOTIFY_RESPONSE_ACCESS \
(FAN_TEST|FAN_ALLOW | FAN_DENY)
...
int access = response & FANOTIFY_RESPONSE_ACCESS;
1. Do return EINVAL for access == 0
2. Let all the rest of the EINVAL checks run (including extra type)
3. Move if (fd < 0) to last check
4. Add if (!access) return 0 before if (fd < 0)
That will provide a mechanism for userspace to probe the
kernel support for extra types in general and specific types
that it may respond with.
> return -EINVAL;
> -
> if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> return -EINVAL;
> -
> + if (response & FAN_EXTRA) {
> + if (count < offsetofend(struct fanotify_response, extra_info_type))
> + return -EINVAL;
> + switch (response_struct->extra_info_type) {
> + case FAN_RESPONSE_INFO_NONE:
> + break;
> + case FAN_RESPONSE_INFO_AUDIT_RULE:
> + if (count < offsetofend(struct fanotify_response, extra_info))
That's a trap right there.
In future kernel, if someone adds a 64bit member to the extra_info union
existing binaries will start failing.
Also since struct fanotify_response is not packed, a 64bit member in the
union will change the alignment of extra_info union.
The use of a union in UAPI seems to be asking for trouble.
You should probably follow the pattern of fanotify_event_info_* structs.
It's more work, but I don't see another way.
> + return -EINVAL;
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> spin_lock(&group->notification_lock);
> list_for_each_entry(event, &group->fanotify_data.access_list,
> fae.fse.list) {
> @@ -340,7 +356,7 @@ 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(group, event, response_struct);
> wake_up(&group->fanotify_data.access_waitq);
> return 0;
> }
> @@ -802,9 +818,13 @@ 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);
> wake_up(&group->fanotify_data.access_waitq);
> } else {
> spin_lock(&group->notification_lock);
> @@ -827,26 +847,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 < offsetofend(struct fanotify_response, 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 (copy_from_user(&response, buf, size))
> return -EFAULT;
>
> - ret = process_access_response(group, &response);
> + ret = process_access_response(group, &response, count);
We did not copy count bytes of response. We copied size bytes.
> if (ret < 0)
> count = ret;
>
> @@ -857,6 +876,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 +898,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);
> spin_lock(&group->notification_lock);
> }
>
> @@ -893,7 +915,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);
> }
> spin_lock(&group->notification_lock);
> }
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index 419cadcd7ff5..63a8494e782e 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -113,6 +113,9 @@
> #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 FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
> +
Please drop this macro and follow the pattern of FANOTIFY_{INIT,MARK,EVENT}_*
#define FANOTIFY_RESPONSE_ACCESS \
(FAN_ALLOW | FAN_DENY)
#define FANOTIFY_RESPONSE_FLAGS \
(FAN_AUDIT | FAN_EXTRA)
#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 e8ac38cc2fd6..a94f4143601f 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -179,15 +179,35 @@ 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
typo: informaion
> + * 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
> +
> +union fanotify_response_extra {
> + __u32 audit_rule;
> +};
> +
> struct fanotify_response {
> __s32 fd;
> __u32 response;
> + __u32 extra_info_type;
> + union fanotify_response_extra extra_info;
IIRC, Jan wanted this to be a variable size record with info_type and info_len.
I don't know if we want to make this flexible enough to allow for multiple
records in the future like we do in events, but the common wisdom of
the universe says that if we don't do it, we will need it.
Thanks,
Amir.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-17 5:37 ` Amir Goldstein
@ 2022-05-17 10:32 ` Jan Kara
-1 siblings, 0 replies; 33+ messages in thread
From: Jan Kara @ 2022-05-17 10:32 UTC (permalink / raw)
To: Amir Goldstein
Cc: Richard Guy Briggs, Linux-Audit Mailing List, LKML,
linux-fsdevel, Paul Moore, Eric Paris, Steve Grubb, Jan Kara
On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
...
> > static int process_access_response(struct fsnotify_group *group,
> > - struct fanotify_response *response_struct)
> > + struct fanotify_response *response_struct,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > - fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > + group, fd, response, response_struct->extra_info_type, count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * 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) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
> However, this behavior change is something that I did ask for, but it should be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I have to admit I didn't quite grok your suggestion here although I
understand (and agree with) the general direction of the proposal :). Maybe
code would explain it better what you have in mind?
> > +/*
> > + * 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
>
> typo: informaion
>
> > + * 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
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and info_len.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
Yes, please no unions in the API, that is always painful with the
alignment, size etc. What I had in mind was:
Keep fanotify_response as is:
struct fanotify_response {
__s32 fd;
__u32 response;
};
Define extra info header:
struct fanotify_response_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
And then struct for your audit rule:
struct fanotify_response_info_audit_rule {
struct fanotify_response_info_header hdr;
__u32 audit_rule;
};
The verification in fanotify_write() then goes like:
struct fanotify_response response;
char extra_info_buf[sizeof(struct fanotify_response_info_audit_rule)];
if (copy_from_user(&response, buf, sizeof(response)))
return -EFAULT;
if (!(response.response & FAN_EXTRA_INFO)) {
count = 0;
} else {
count -= sizeof(response);
/* Simplistic parsing for now */
if (count != sizeof(struct fanotify_response_info_audit_rule))
return -EINVAL;
if (copy_from_user(extra_info_buf, buf, count)
return -EFAULT;
}
ret = process_access_response(group, &response, extra_info_buf, count);
And we pass extra_info_buf and count to audit_fanotify() where we need to do
further validation like:
struct fanotify_response_info_audit_rule *audit_response = NULL;
if (count > 0) {
/* Just one possible info type for now */
audit_response = (struct fanotify_response_info_audit_rule *)extra_info_buf;
if (audit_response->info_type != FAN_RESPONSE_INFO_AUDIT_RULE)
return -EINVAL;
if (audit_response->pad != 0)
return -EINVAL;
if (audit_response->len != sizeof(*audit_response))
return -EINVAL;
}
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-17 10:32 ` Jan Kara
0 siblings, 0 replies; 33+ messages in thread
From: Jan Kara @ 2022-05-17 10:32 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, Richard Guy Briggs, LKML, Linux-Audit Mailing List,
linux-fsdevel, Eric Paris
On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
...
> > static int process_access_response(struct fsnotify_group *group,
> > - struct fanotify_response *response_struct)
> > + struct fanotify_response *response_struct,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > - fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > + group, fd, response, response_struct->extra_info_type, count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * 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) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
> However, this behavior change is something that I did ask for, but it should be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I have to admit I didn't quite grok your suggestion here although I
understand (and agree with) the general direction of the proposal :). Maybe
code would explain it better what you have in mind?
> > +/*
> > + * 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
>
> typo: informaion
>
> > + * 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
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and info_len.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
Yes, please no unions in the API, that is always painful with the
alignment, size etc. What I had in mind was:
Keep fanotify_response as is:
struct fanotify_response {
__s32 fd;
__u32 response;
};
Define extra info header:
struct fanotify_response_info_header {
__u8 info_type;
__u8 pad;
__u16 len;
};
And then struct for your audit rule:
struct fanotify_response_info_audit_rule {
struct fanotify_response_info_header hdr;
__u32 audit_rule;
};
The verification in fanotify_write() then goes like:
struct fanotify_response response;
char extra_info_buf[sizeof(struct fanotify_response_info_audit_rule)];
if (copy_from_user(&response, buf, sizeof(response)))
return -EFAULT;
if (!(response.response & FAN_EXTRA_INFO)) {
count = 0;
} else {
count -= sizeof(response);
/* Simplistic parsing for now */
if (count != sizeof(struct fanotify_response_info_audit_rule))
return -EINVAL;
if (copy_from_user(extra_info_buf, buf, count)
return -EFAULT;
}
ret = process_access_response(group, &response, extra_info_buf, count);
And we pass extra_info_buf and count to audit_fanotify() where we need to do
further validation like:
struct fanotify_response_info_audit_rule *audit_response = NULL;
if (count > 0) {
/* Just one possible info type for now */
audit_response = (struct fanotify_response_info_audit_rule *)extra_info_buf;
if (audit_response->info_type != FAN_RESPONSE_INFO_AUDIT_RULE)
return -EINVAL;
if (audit_response->pad != 0)
return -EINVAL;
if (audit_response->len != sizeof(*audit_response))
return -EINVAL;
}
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-17 10:32 ` Jan Kara
@ 2022-05-17 11:31 ` Amir Goldstein
-1 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-17 11:31 UTC (permalink / raw)
To: Jan Kara
Cc: Richard Guy Briggs, Linux-Audit Mailing List, LKML,
linux-fsdevel, Paul Moore, Eric Paris, Steve Grubb
On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
>
> ...
> > > static int process_access_response(struct fsnotify_group *group,
> > > - struct fanotify_response *response_struct)
> > > + struct fanotify_response *response_struct,
> > > + size_t count)
> > > {
> > > struct fanotify_perm_event *event;
> > > int fd = response_struct->fd;
> > > u32 response = response_struct->response;
> > >
> > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > - fd, response);
> > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > + group, fd, response, response_struct->extra_info_type, count);
> > > + if (fd < 0)
> > > + return -EINVAL;
> > > /*
> > > * 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) {
> > > - case FAN_ALLOW:
> > > - case FAN_DENY:
> > > - break;
> > > - default:
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (fd < 0)
> > > + if (FAN_INVALID_RESPONSE_MASK(response))
> >
> > That is a logic change, because now the response value of 0 becomes valid.
> >
> > Since you did not document this change in the commit message I assume this was
> > non intentional?
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I have to admit I didn't quite grok your suggestion here although I
> understand (and agree with) the general direction of the proposal :). Maybe
> code would explain it better what you have in mind?
>
+/* These are NOT bitwise flags. Both bits can be used together. */
+#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
#define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
+#define FANOTIFY_RESPONSE_ACCESS \
+ (FAN_TEST|FAN_ALLOW | FAN_DENY)
...
@@ -311,6 +314,7 @@ static int process_access_response(struct
fsnotify_group *group,
struct fanotify_perm_event *event;
int fd = response_struct->fd;
int response = response_struct->response;
+ int access = response & FANOTIFY_RESPONSE_ACCESS;
pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
fd, response);
@@ -319,18 +323,33 @@ static int process_access_response(struct
fsnotify_group *group,
* userspace can send a valid response or we will clean it up after the
* timeout
*/
- switch (response & ~FAN_AUDIT) {
+ if (!response)
+ return -EINVAL;
+
+ switch (access) {
case FAN_ALLOW:
case FAN_DENY:
+ case FAN_TEST:
break;
default:
return -EINVAL;
}
- if (fd < 0)
- return -EINVAL;
-
if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
+ /*
+ * FAN_TEST|FAN_AUDIT response can be written during setup time to probe
+ * if the kernel has support for FAN_AUDIT.
+ * For FAN_TEST, fd must not be valid.
+ */
+ if (access == FAN_TEST) {
+ if (fd >= 0)
+ return -EINVAL;
+ return 0;
+ }
+
+ if (fd < 0)
+ return -EINVAL;
Thanks,
Amir.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-17 11:31 ` Amir Goldstein
0 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-17 11:31 UTC (permalink / raw)
To: Jan Kara
Cc: Richard Guy Briggs, LKML, Linux-Audit Mailing List,
linux-fsdevel, Eric Paris
On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
>
> ...
> > > static int process_access_response(struct fsnotify_group *group,
> > > - struct fanotify_response *response_struct)
> > > + struct fanotify_response *response_struct,
> > > + size_t count)
> > > {
> > > struct fanotify_perm_event *event;
> > > int fd = response_struct->fd;
> > > u32 response = response_struct->response;
> > >
> > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > - fd, response);
> > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > + group, fd, response, response_struct->extra_info_type, count);
> > > + if (fd < 0)
> > > + return -EINVAL;
> > > /*
> > > * 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) {
> > > - case FAN_ALLOW:
> > > - case FAN_DENY:
> > > - break;
> > > - default:
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (fd < 0)
> > > + if (FAN_INVALID_RESPONSE_MASK(response))
> >
> > That is a logic change, because now the response value of 0 becomes valid.
> >
> > Since you did not document this change in the commit message I assume this was
> > non intentional?
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I have to admit I didn't quite grok your suggestion here although I
> understand (and agree with) the general direction of the proposal :). Maybe
> code would explain it better what you have in mind?
>
+/* These are NOT bitwise flags. Both bits can be used together. */
+#define FAN_TEST 0x00
#define FAN_ALLOW 0x01
#define FAN_DENY 0x02
#define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
+#define FANOTIFY_RESPONSE_ACCESS \
+ (FAN_TEST|FAN_ALLOW | FAN_DENY)
...
@@ -311,6 +314,7 @@ static int process_access_response(struct
fsnotify_group *group,
struct fanotify_perm_event *event;
int fd = response_struct->fd;
int response = response_struct->response;
+ int access = response & FANOTIFY_RESPONSE_ACCESS;
pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
fd, response);
@@ -319,18 +323,33 @@ static int process_access_response(struct
fsnotify_group *group,
* userspace can send a valid response or we will clean it up after the
* timeout
*/
- switch (response & ~FAN_AUDIT) {
+ if (!response)
+ return -EINVAL;
+
+ switch (access) {
case FAN_ALLOW:
case FAN_DENY:
+ case FAN_TEST:
break;
default:
return -EINVAL;
}
- if (fd < 0)
- return -EINVAL;
-
if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
return -EINVAL;
+ /*
+ * FAN_TEST|FAN_AUDIT response can be written during setup time to probe
+ * if the kernel has support for FAN_AUDIT.
+ * For FAN_TEST, fd must not be valid.
+ */
+ if (access == FAN_TEST) {
+ if (fd >= 0)
+ return -EINVAL;
+ return 0;
+ }
+
+ if (fd < 0)
+ return -EINVAL;
Thanks,
Amir.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-17 11:31 ` Amir Goldstein
@ 2022-05-17 12:06 ` Amir Goldstein
-1 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-17 12:06 UTC (permalink / raw)
To: Jan Kara
Cc: Richard Guy Briggs, Linux-Audit Mailing List, LKML,
linux-fsdevel, Paul Moore, Eric Paris, Steve Grubb
On Tue, May 17, 2022 at 2:31 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > > On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
> >
> > ...
> > > > static int process_access_response(struct fsnotify_group *group,
> > > > - struct fanotify_response *response_struct)
> > > > + struct fanotify_response *response_struct,
> > > > + size_t count)
> > > > {
> > > > struct fanotify_perm_event *event;
> > > > int fd = response_struct->fd;
> > > > u32 response = response_struct->response;
> > > >
> > > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > > - fd, response);
> > > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > > + group, fd, response, response_struct->extra_info_type, count);
> > > > + if (fd < 0)
> > > > + return -EINVAL;
> > > > /*
> > > > * 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) {
> > > > - case FAN_ALLOW:
> > > > - case FAN_DENY:
> > > > - break;
> > > > - default:
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - if (fd < 0)
> > > > + if (FAN_INVALID_RESPONSE_MASK(response))
> > >
> > > That is a logic change, because now the response value of 0 becomes valid.
> > >
> > > Since you did not document this change in the commit message I assume this was
> > > non intentional?
> > > However, this behavior change is something that I did ask for, but it should be
> > > done is a separate commit:
> > >
> > > /* These are NOT bitwise flags. Both bits can be used together. */
> > > #define FAN_TEST 0x00
> > > #define FAN_ALLOW 0x01
> > > #define FAN_DENY 0x02
> > > #define FANOTIFY_RESPONSE_ACCESS \
> > > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> > >
> > > ...
> > > int access = response & FANOTIFY_RESPONSE_ACCESS;
> > >
> > > 1. Do return EINVAL for access == 0
> > > 2. Let all the rest of the EINVAL checks run (including extra type)
> > > 3. Move if (fd < 0) to last check
> > > 4. Add if (!access) return 0 before if (fd < 0)
> > >
> > > That will provide a mechanism for userspace to probe the
> > > kernel support for extra types in general and specific types
> > > that it may respond with.
> >
> > I have to admit I didn't quite grok your suggestion here although I
> > understand (and agree with) the general direction of the proposal :). Maybe
> > code would explain it better what you have in mind?
> >
>
> +/* These are NOT bitwise flags. Both bits can be used together. */
I realize when reading this that this comment is weird, because
0x01 and 0x02 cannot currently be used together.
The comment was copied from above FAN_MARK_INODE where it
has the same weirdness.
The meaning is that (response & FANOTIFY_RESPONSE_ACCESS)
is an enum. I am sure that a less confusing phrasing for this comment
can be found.
> +#define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
> +#define FANOTIFY_RESPONSE_ACCESS \
> + (FAN_TEST|FAN_ALLOW | FAN_DENY)
Thanks,
Amir.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-17 12:06 ` Amir Goldstein
0 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-17 12:06 UTC (permalink / raw)
To: Jan Kara
Cc: Richard Guy Briggs, LKML, Linux-Audit Mailing List,
linux-fsdevel, Eric Paris
On Tue, May 17, 2022 at 2:31 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, May 17, 2022 at 1:32 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> > > On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
> >
> > ...
> > > > static int process_access_response(struct fsnotify_group *group,
> > > > - struct fanotify_response *response_struct)
> > > > + struct fanotify_response *response_struct,
> > > > + size_t count)
> > > > {
> > > > struct fanotify_perm_event *event;
> > > > int fd = response_struct->fd;
> > > > u32 response = response_struct->response;
> > > >
> > > > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > > > - fd, response);
> > > > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > > > + group, fd, response, response_struct->extra_info_type, count);
> > > > + if (fd < 0)
> > > > + return -EINVAL;
> > > > /*
> > > > * 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) {
> > > > - case FAN_ALLOW:
> > > > - case FAN_DENY:
> > > > - break;
> > > > - default:
> > > > - return -EINVAL;
> > > > - }
> > > > -
> > > > - if (fd < 0)
> > > > + if (FAN_INVALID_RESPONSE_MASK(response))
> > >
> > > That is a logic change, because now the response value of 0 becomes valid.
> > >
> > > Since you did not document this change in the commit message I assume this was
> > > non intentional?
> > > However, this behavior change is something that I did ask for, but it should be
> > > done is a separate commit:
> > >
> > > /* These are NOT bitwise flags. Both bits can be used together. */
> > > #define FAN_TEST 0x00
> > > #define FAN_ALLOW 0x01
> > > #define FAN_DENY 0x02
> > > #define FANOTIFY_RESPONSE_ACCESS \
> > > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> > >
> > > ...
> > > int access = response & FANOTIFY_RESPONSE_ACCESS;
> > >
> > > 1. Do return EINVAL for access == 0
> > > 2. Let all the rest of the EINVAL checks run (including extra type)
> > > 3. Move if (fd < 0) to last check
> > > 4. Add if (!access) return 0 before if (fd < 0)
> > >
> > > That will provide a mechanism for userspace to probe the
> > > kernel support for extra types in general and specific types
> > > that it may respond with.
> >
> > I have to admit I didn't quite grok your suggestion here although I
> > understand (and agree with) the general direction of the proposal :). Maybe
> > code would explain it better what you have in mind?
> >
>
> +/* These are NOT bitwise flags. Both bits can be used together. */
I realize when reading this that this comment is weird, because
0x01 and 0x02 cannot currently be used together.
The comment was copied from above FAN_MARK_INODE where it
has the same weirdness.
The meaning is that (response & FANOTIFY_RESPONSE_ACCESS)
is an enum. I am sure that a less confusing phrasing for this comment
can be found.
> +#define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FAN_AUDIT 0x10 /* Bit mask to create audit record for result */
> +#define FANOTIFY_RESPONSE_ACCESS \
> + (FAN_TEST|FAN_ALLOW | FAN_DENY)
Thanks,
Amir.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-17 5:37 ` Amir Goldstein
@ 2022-05-19 0:07 ` Richard Guy Briggs
-1 siblings, 0 replies; 33+ messages in thread
From: Richard Guy Briggs @ 2022-05-19 0:07 UTC (permalink / raw)
To: Amir Goldstein
Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, Paul Moore,
Eric Paris, Steve Grubb, Jan Kara
On 2022-05-17 08:37, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
> > ---
> > fs/notify/fanotify/fanotify.c | 2 +-
> > fs/notify/fanotify/fanotify.h | 2 +
> > fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
> > include/linux/fanotify.h | 3 ++
> > include/uapi/linux/fanotify.h | 22 ++++++++-
> > 5 files changed, 75 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 985e995d2a39..ea0e60488f12 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
> > }
> >
> > /* userspace responded, convert to something usable */
> > - switch (event->response & ~FAN_AUDIT) {
> > + switch (event->response & ~(FAN_AUDIT | FAN_EXTRA)) {
> > case FAN_ALLOW:
> > ret = 0;
> > break;
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index d66668e06bee..eb7ec1f2a26e 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> > struct fanotify_event fae;
> > struct path path;
> > u32 response; /* userspace answer to the event */
> > + u32 extra_info_type;
> > unsigned short state; /* state of the event */
> > int fd; /* fd we passed to userspace for this event */
> > + union fanotify_response_extra extra_info;
> > };
> >
> > static inline struct fanotify_perm_event *
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 721e777ea90b..1c4067e29f2e 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -289,13 +289,22 @@ 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 & ~FAN_EXTRA;
> > + if (response->response & FAN_EXTRA) {
> > + event->extra_info_type = response->extra_info_type;
> > + switch (event->extra_info_type) {
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + event->extra_info.audit_rule = response->extra_info.audit_rule;
> > + }
> > + } else {
> > + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> > + }
> > if (event->state == FAN_EVENT_CANCELED)
> > destroy = true;
> > else
> > @@ -306,33 +315,40 @@ 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,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > - fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > + group, fd, response, response_struct->extra_info_type, count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * 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) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
It was not intentional. In hindsight, I should have restored the
original code, or at least looked at the original much more carefully to
duplicate its behaviour.
> However, this behavior change is something that I did ask for, but it should be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
Going back to the original code will do that.
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I'm still resisting the idea of the TEST flag... It seems like an
unneeded extra step and complication...
The simple presence of the FAN_EXTRA flag should sort it out and could
even make TEST one of the types.
> > return -EINVAL;
> > -
> > if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > return -EINVAL;
> > -
> > + if (response & FAN_EXTRA) {
> > + if (count < offsetofend(struct fanotify_response, extra_info_type))
> > + return -EINVAL;
> > + switch (response_struct->extra_info_type) {
> > + case FAN_RESPONSE_INFO_NONE:
> > + break;
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + if (count < offsetofend(struct fanotify_response, extra_info))
>
> That's a trap right there.
> In future kernel, if someone adds a 64bit member to the extra_info union
> existing binaries will start failing.
In hindsight, agreed. It should have aimed for the end of "__u32
audit_rule" for FAN_RESPONSE_INFO_AUDIT_RULE.
> Also since struct fanotify_response is not packed, a 64bit member in the
> union will change the alignment of extra_info union.
> The use of a union in UAPI seems to be asking for trouble.
I'll have to take your word for it.
> You should probably follow the pattern of fanotify_event_info_* structs.
> It's more work, but I don't see another way.
I was thinking this would be fine until it was expanded and could be
separated then, but the issue above demonstrates that is false.
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > spin_lock(&group->notification_lock);
> > list_for_each_entry(event, &group->fanotify_data.access_list,
> > fae.fse.list) {
> > @@ -340,7 +356,7 @@ 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(group, event, response_struct);
> > wake_up(&group->fanotify_data.access_waitq);
> > return 0;
> > }
> > @@ -802,9 +818,13 @@ 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);
> > wake_up(&group->fanotify_data.access_waitq);
> > } else {
> > spin_lock(&group->notification_lock);
> > @@ -827,26 +847,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 < offsetofend(struct fanotify_response, 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 (copy_from_user(&response, buf, size))
> > return -EFAULT;
> >
> > - ret = process_access_response(group, &response);
> > + ret = process_access_response(group, &response, count);
>
> We did not copy count bytes of response. We copied size bytes.
This was intentional as a safeguard to not overflow the struct, but also
not take garbage from userspace. If it is an old userspace, the padding
is blank and meaningless. If userspace sends more, it won't trample
beyond the struct. The types involved would take care of that later.
> > if (ret < 0)
> > count = ret;
> >
> > @@ -857,6 +876,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 +898,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);
> > spin_lock(&group->notification_lock);
> > }
> >
> > @@ -893,7 +915,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);
> > }
> > spin_lock(&group->notification_lock);
> > }
> > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > index 419cadcd7ff5..63a8494e782e 100644
> > --- a/include/linux/fanotify.h
> > +++ b/include/linux/fanotify.h
> > @@ -113,6 +113,9 @@
> > #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 FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
> > +
>
> Please drop this macro and follow the pattern of FANOTIFY_{INIT,MARK,EVENT}_*
>
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_ALLOW | FAN_DENY)
> #define FANOTIFY_RESPONSE_FLAGS \
> (FAN_AUDIT | FAN_EXTRA)
> #define FANOTIFY_RESPONSE_VALID_MASK \
> (FANOTIFY_RESPONSE_ACCESS | \
> FANOTIFY_RESPONSE_FLAGS)
This seems like a reasonable approach.
> > /* 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 e8ac38cc2fd6..a94f4143601f 100644
> > --- a/include/uapi/linux/fanotify.h
> > +++ b/include/uapi/linux/fanotify.h
> > @@ -179,15 +179,35 @@ 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
>
> typo: informaion
Thanks.
> > + * 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
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and info_len.
Again, the intent was to make it fixed for now and change it later if
needed, but that was a shortsighted approach...
I don't see a need for a len in all response types. _NONE doesn't need
any. _AUDIT_RULE is known to be 32 bits. Other types can define their
size and layout as needed, including a len field if it is needed.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
It did occur to me that this could be used for other than audit, hence
the renaming of the ..."_NONE" macro.
We should be able in the future to define a type that is extensible or
has multiple records. We have (2^32) - 2 types left to work with.
> Thanks,
> Amir.
- RGB
--
Richard Guy Briggs <rgb@redhat.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
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-19 0:07 ` Richard Guy Briggs
0 siblings, 0 replies; 33+ messages in thread
From: Richard Guy Briggs @ 2022-05-19 0:07 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, LKML, Linux-Audit Mailing List, linux-fsdevel, Eric Paris
On 2022-05-17 08:37, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 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 32 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 union containing 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>
> > ---
> > fs/notify/fanotify/fanotify.c | 2 +-
> > fs/notify/fanotify/fanotify.h | 2 +
> > fs/notify/fanotify/fanotify_user.c | 74 +++++++++++++++++++-----------
> > include/linux/fanotify.h | 3 ++
> > include/uapi/linux/fanotify.h | 22 ++++++++-
> > 5 files changed, 75 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> > index 985e995d2a39..ea0e60488f12 100644
> > --- a/fs/notify/fanotify/fanotify.c
> > +++ b/fs/notify/fanotify/fanotify.c
> > @@ -262,7 +262,7 @@ static int fanotify_get_response(struct fsnotify_group *group,
> > }
> >
> > /* userspace responded, convert to something usable */
> > - switch (event->response & ~FAN_AUDIT) {
> > + switch (event->response & ~(FAN_AUDIT | FAN_EXTRA)) {
> > case FAN_ALLOW:
> > ret = 0;
> > break;
> > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> > index d66668e06bee..eb7ec1f2a26e 100644
> > --- a/fs/notify/fanotify/fanotify.h
> > +++ b/fs/notify/fanotify/fanotify.h
> > @@ -426,8 +426,10 @@ struct fanotify_perm_event {
> > struct fanotify_event fae;
> > struct path path;
> > u32 response; /* userspace answer to the event */
> > + u32 extra_info_type;
> > unsigned short state; /* state of the event */
> > int fd; /* fd we passed to userspace for this event */
> > + union fanotify_response_extra extra_info;
> > };
> >
> > static inline struct fanotify_perm_event *
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 721e777ea90b..1c4067e29f2e 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -289,13 +289,22 @@ 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 & ~FAN_EXTRA;
> > + if (response->response & FAN_EXTRA) {
> > + event->extra_info_type = response->extra_info_type;
> > + switch (event->extra_info_type) {
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + event->extra_info.audit_rule = response->extra_info.audit_rule;
> > + }
> > + } else {
> > + event->extra_info_type = FAN_RESPONSE_INFO_NONE;
> > + }
> > if (event->state == FAN_EVENT_CANCELED)
> > destroy = true;
> > else
> > @@ -306,33 +315,40 @@ 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,
> > + size_t count)
> > {
> > struct fanotify_perm_event *event;
> > int fd = response_struct->fd;
> > u32 response = response_struct->response;
> >
> > - pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > - fd, response);
> > + pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > + group, fd, response, response_struct->extra_info_type, count);
> > + if (fd < 0)
> > + return -EINVAL;
> > /*
> > * 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) {
> > - case FAN_ALLOW:
> > - case FAN_DENY:
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > -
> > - if (fd < 0)
> > + if (FAN_INVALID_RESPONSE_MASK(response))
>
> That is a logic change, because now the response value of 0 becomes valid.
>
> Since you did not document this change in the commit message I assume this was
> non intentional?
It was not intentional. In hindsight, I should have restored the
original code, or at least looked at the original much more carefully to
duplicate its behaviour.
> However, this behavior change is something that I did ask for, but it should be
> done is a separate commit:
>
> /* These are NOT bitwise flags. Both bits can be used together. */
> #define FAN_TEST 0x00
> #define FAN_ALLOW 0x01
> #define FAN_DENY 0x02
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_TEST|FAN_ALLOW | FAN_DENY)
>
> ...
> int access = response & FANOTIFY_RESPONSE_ACCESS;
>
> 1. Do return EINVAL for access == 0
Going back to the original code will do that.
> 2. Let all the rest of the EINVAL checks run (including extra type)
> 3. Move if (fd < 0) to last check
> 4. Add if (!access) return 0 before if (fd < 0)
>
> That will provide a mechanism for userspace to probe the
> kernel support for extra types in general and specific types
> that it may respond with.
I'm still resisting the idea of the TEST flag... It seems like an
unneeded extra step and complication...
The simple presence of the FAN_EXTRA flag should sort it out and could
even make TEST one of the types.
> > return -EINVAL;
> > -
> > if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> > return -EINVAL;
> > -
> > + if (response & FAN_EXTRA) {
> > + if (count < offsetofend(struct fanotify_response, extra_info_type))
> > + return -EINVAL;
> > + switch (response_struct->extra_info_type) {
> > + case FAN_RESPONSE_INFO_NONE:
> > + break;
> > + case FAN_RESPONSE_INFO_AUDIT_RULE:
> > + if (count < offsetofend(struct fanotify_response, extra_info))
>
> That's a trap right there.
> In future kernel, if someone adds a 64bit member to the extra_info union
> existing binaries will start failing.
In hindsight, agreed. It should have aimed for the end of "__u32
audit_rule" for FAN_RESPONSE_INFO_AUDIT_RULE.
> Also since struct fanotify_response is not packed, a 64bit member in the
> union will change the alignment of extra_info union.
> The use of a union in UAPI seems to be asking for trouble.
I'll have to take your word for it.
> You should probably follow the pattern of fanotify_event_info_* structs.
> It's more work, but I don't see another way.
I was thinking this would be fine until it was expanded and could be
separated then, but the issue above demonstrates that is false.
> > + return -EINVAL;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > + }
> > spin_lock(&group->notification_lock);
> > list_for_each_entry(event, &group->fanotify_data.access_list,
> > fae.fse.list) {
> > @@ -340,7 +356,7 @@ 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(group, event, response_struct);
> > wake_up(&group->fanotify_data.access_waitq);
> > return 0;
> > }
> > @@ -802,9 +818,13 @@ 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);
> > wake_up(&group->fanotify_data.access_waitq);
> > } else {
> > spin_lock(&group->notification_lock);
> > @@ -827,26 +847,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 < offsetofend(struct fanotify_response, 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 (copy_from_user(&response, buf, size))
> > return -EFAULT;
> >
> > - ret = process_access_response(group, &response);
> > + ret = process_access_response(group, &response, count);
>
> We did not copy count bytes of response. We copied size bytes.
This was intentional as a safeguard to not overflow the struct, but also
not take garbage from userspace. If it is an old userspace, the padding
is blank and meaningless. If userspace sends more, it won't trample
beyond the struct. The types involved would take care of that later.
> > if (ret < 0)
> > count = ret;
> >
> > @@ -857,6 +876,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 +898,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);
> > spin_lock(&group->notification_lock);
> > }
> >
> > @@ -893,7 +915,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);
> > }
> > spin_lock(&group->notification_lock);
> > }
> > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> > index 419cadcd7ff5..63a8494e782e 100644
> > --- a/include/linux/fanotify.h
> > +++ b/include/linux/fanotify.h
> > @@ -113,6 +113,9 @@
> > #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 FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | FAN_AUDIT | FAN_EXTRA))
> > +
>
> Please drop this macro and follow the pattern of FANOTIFY_{INIT,MARK,EVENT}_*
>
> #define FANOTIFY_RESPONSE_ACCESS \
> (FAN_ALLOW | FAN_DENY)
> #define FANOTIFY_RESPONSE_FLAGS \
> (FAN_AUDIT | FAN_EXTRA)
> #define FANOTIFY_RESPONSE_VALID_MASK \
> (FANOTIFY_RESPONSE_ACCESS | \
> FANOTIFY_RESPONSE_FLAGS)
This seems like a reasonable approach.
> > /* 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 e8ac38cc2fd6..a94f4143601f 100644
> > --- a/include/uapi/linux/fanotify.h
> > +++ b/include/uapi/linux/fanotify.h
> > @@ -179,15 +179,35 @@ 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
>
> typo: informaion
Thanks.
> > + * 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
> > +
> > +union fanotify_response_extra {
> > + __u32 audit_rule;
> > +};
> > +
> > struct fanotify_response {
> > __s32 fd;
> > __u32 response;
> > + __u32 extra_info_type;
> > + union fanotify_response_extra extra_info;
>
> IIRC, Jan wanted this to be a variable size record with info_type and info_len.
Again, the intent was to make it fixed for now and change it later if
needed, but that was a shortsighted approach...
I don't see a need for a len in all response types. _NONE doesn't need
any. _AUDIT_RULE is known to be 32 bits. Other types can define their
size and layout as needed, including a len field if it is needed.
> I don't know if we want to make this flexible enough to allow for multiple
> records in the future like we do in events, but the common wisdom of
> the universe says that if we don't do it, we will need it.
It did occur to me that this could be used for other than audit, hence
the renaming of the ..."_NONE" macro.
We should be able in the future to define a type that is extensible or
has multiple records. We have (2^32) - 2 types left to work with.
> Thanks,
> Amir.
- RGB
--
Richard Guy Briggs <rgb@redhat.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
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-19 0:07 ` Richard Guy Briggs
@ 2022-05-19 6:03 ` Amir Goldstein
-1 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-19 6:03 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, Paul Moore,
Eric Paris, Steve Grubb, Jan Kara
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
>
> Going back to the original code will do that.
Oops, this was supposed to be Do NOT return EINVAL for access == 0
this is the case of FAN_TEST.
The patch I posted later explains that better.
>
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I'm still resisting the idea of the TEST flag... It seems like an
> unneeded extra step and complication...
Please reply to the patch I posted as a reply as point
at said complication. There is no extra step.
>
> The simple presence of the FAN_EXTRA flag should sort it out and could
> even make TEST one of the types.
>
I think you've missed the point of the TEST response code.
The point of the TEST response code is to test whether the
extra type is supported, so TESTS cannot be a type.
You should not think of FAN_TEST as a flag at all, in
fact, it is semantic and can be omitted altogether.
The core of the idea is that:
int access = response & FANOTIFY_RESPONSE_ACCESS;
access is an enum, not a bitwise mask, much like:
unsigned int class = flags & FANOTIFY_CLASS_BITS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
At the moment, userspace must provide a valid access code
either ALLOW or DENY.
Providing no access code (0) is not valid.
I suggest making FAN_EXTRA with no access code a valid
response for testing the EXTRA types support.
(please refer to the patch)
[...]
> > > + * 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
> > > +
> > > +union fanotify_response_extra {
> > > + __u32 audit_rule;
> > > +};
> > > +
> > > struct fanotify_response {
> > > __s32 fd;
> > > __u32 response;
> > > + __u32 extra_info_type;
> > > + union fanotify_response_extra extra_info;
> >
> > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
>
> Again, the intent was to make it fixed for now and change it later if
> needed, but that was a shortsighted approach...
>
> I don't see a need for a len in all response types. _NONE doesn't need
> any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> size and layout as needed, including a len field if it is needed.
>
len is part of a common response info header.
It is meant to make writing generic code.
So Jan's email.
> > I don't know if we want to make this flexible enough to allow for multiple
> > records in the future like we do in events, but the common wisdom of
> > the universe says that if we don't do it, we will need it.
>
> It did occur to me that this could be used for other than audit, hence
> the renaming of the ..."_NONE" macro.
>
> We should be able in the future to define a type that is extensible or
> has multiple records. We have (2^32) - 2 types left to work with.
>
The way this was done when we first introduced event info
records was the same. We only allowed one type of record
and a single record to begin with, but the format allowed for
extending to multiple records.
struct fanotify_event_metadata already had event_len and
metadata_len, so that was convenient. Supporting multi
records only required that every record has a header with its
own len.
As far as I can tell, the case of fanotify_response is different
because we have the count argument of write(), which serves
as the total response_len.
If we ever want to be able to extend the base fanotify_response,
add fields to it not as extra info records, then we need to add
response_metadata_len to struct fanotify_response, but I think that
would be over design.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-19 6:03 ` Amir Goldstein
0 siblings, 0 replies; 33+ messages in thread
From: Amir Goldstein @ 2022-05-19 6:03 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Jan Kara, LKML, Linux-Audit Mailing List, linux-fsdevel, Eric Paris
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
>
> Going back to the original code will do that.
Oops, this was supposed to be Do NOT return EINVAL for access == 0
this is the case of FAN_TEST.
The patch I posted later explains that better.
>
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I'm still resisting the idea of the TEST flag... It seems like an
> unneeded extra step and complication...
Please reply to the patch I posted as a reply as point
at said complication. There is no extra step.
>
> The simple presence of the FAN_EXTRA flag should sort it out and could
> even make TEST one of the types.
>
I think you've missed the point of the TEST response code.
The point of the TEST response code is to test whether the
extra type is supported, so TESTS cannot be a type.
You should not think of FAN_TEST as a flag at all, in
fact, it is semantic and can be omitted altogether.
The core of the idea is that:
int access = response & FANOTIFY_RESPONSE_ACCESS;
access is an enum, not a bitwise mask, much like:
unsigned int class = flags & FANOTIFY_CLASS_BITS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
At the moment, userspace must provide a valid access code
either ALLOW or DENY.
Providing no access code (0) is not valid.
I suggest making FAN_EXTRA with no access code a valid
response for testing the EXTRA types support.
(please refer to the patch)
[...]
> > > + * 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
> > > +
> > > +union fanotify_response_extra {
> > > + __u32 audit_rule;
> > > +};
> > > +
> > > struct fanotify_response {
> > > __s32 fd;
> > > __u32 response;
> > > + __u32 extra_info_type;
> > > + union fanotify_response_extra extra_info;
> >
> > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
>
> Again, the intent was to make it fixed for now and change it later if
> needed, but that was a shortsighted approach...
>
> I don't see a need for a len in all response types. _NONE doesn't need
> any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> size and layout as needed, including a len field if it is needed.
>
len is part of a common response info header.
It is meant to make writing generic code.
So Jan's email.
> > I don't know if we want to make this flexible enough to allow for multiple
> > records in the future like we do in events, but the common wisdom of
> > the universe says that if we don't do it, we will need it.
>
> It did occur to me that this could be used for other than audit, hence
> the renaming of the ..."_NONE" macro.
>
> We should be able in the future to define a type that is extensible or
> has multiple records. We have (2^32) - 2 types left to work with.
>
The way this was done when we first introduced event info
records was the same. We only allowed one type of record
and a single record to begin with, but the format allowed for
extending to multiple records.
struct fanotify_event_metadata already had event_len and
metadata_len, so that was convenient. Supporting multi
records only required that every record has a header with its
own len.
As far as I can tell, the case of fanotify_response is different
because we have the count argument of write(), which serves
as the total response_len.
If we ever want to be able to extend the base fanotify_response,
add fields to it not as extra info records, then we need to add
response_metadata_len to struct fanotify_response, but I think that
would be over design.
Thanks,
Amir.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-19 6:03 ` Amir Goldstein
@ 2022-05-19 9:55 ` Jan Kara
-1 siblings, 0 replies; 33+ messages in thread
From: Jan Kara @ 2022-05-19 9:55 UTC (permalink / raw)
To: Amir Goldstein
Cc: Richard Guy Briggs, Linux-Audit Mailing List, LKML,
linux-fsdevel, Paul Moore, Eric Paris, Steve Grubb, Jan Kara
On Thu 19-05-22 09:03:51, Amir Goldstein wrote:
> > > > + * 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
> > > > +
> > > > +union fanotify_response_extra {
> > > > + __u32 audit_rule;
> > > > +};
> > > > +
> > > > struct fanotify_response {
> > > > __s32 fd;
> > > > __u32 response;
> > > > + __u32 extra_info_type;
> > > > + union fanotify_response_extra extra_info;
> > >
> > > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
> >
> > Again, the intent was to make it fixed for now and change it later if
> > needed, but that was a shortsighted approach...
> >
> > I don't see a need for a len in all response types. _NONE doesn't need
> > any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> > size and layout as needed, including a len field if it is needed.
> >
>
> len is part of a common response info header.
> It is meant to make writing generic code.
> So Jan's email.
Yes. The reason why I want 'type' + 'len' information for every extra
response type is so that the code can be layered properly. Fanotify has no
bussiness in understanding the details of the additional info (or its
expected length) passed from userspace. That is the knowledge that should
stay within the subsystem this info is for. So the length of info record
needs to be passed in the generic info header.
To give an example imagine a situation when we'd like to attach two
different info records to a response, each for a different subsystem. Then
fanotify has to split response buffer and pass each info to the target
subsystem or maybe we'd just pass all info to both subsystems and define
they should ignore info they don't understand but in either case we need to
have a way to be able to separate different info records without apriori
knowledge what they actually mean or what is their expected length.
> > > I don't know if we want to make this flexible enough to allow for multiple
> > > records in the future like we do in events, but the common wisdom of
> > > the universe says that if we don't do it, we will need it.
> >
> > It did occur to me that this could be used for other than audit, hence
> > the renaming of the ..."_NONE" macro.
> >
> > We should be able in the future to define a type that is extensible or
> > has multiple records. We have (2^32) - 2 types left to work with.
> >
>
> The way this was done when we first introduced event info
> records was the same. We only allowed one type of record
> and a single record to begin with, but the format allowed for
> extending to multiple records.
>
> struct fanotify_event_metadata already had event_len and
> metadata_len, so that was convenient. Supporting multi
> records only required that every record has a header with its
> own len.
>
> As far as I can tell, the case of fanotify_response is different
> because we have the count argument of write(), which serves
> as the total response_len.
Yes.
> If we ever want to be able to extend the base fanotify_response,
> add fields to it not as extra info records, then we need to add
> response_metadata_len to struct fanotify_response, but I think that
> would be over design.
Yeah, I don't think that will happen. The standard response metadata is
basically fixed by backward compatibility constraints. If we need to extend
it in the future, I would prefer the extension to be in a form of an extra
info record.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-19 9:55 ` Jan Kara
0 siblings, 0 replies; 33+ messages in thread
From: Jan Kara @ 2022-05-19 9:55 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jan Kara, Richard Guy Briggs, LKML, Linux-Audit Mailing List,
linux-fsdevel, Eric Paris
On Thu 19-05-22 09:03:51, Amir Goldstein wrote:
> > > > + * 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
> > > > +
> > > > +union fanotify_response_extra {
> > > > + __u32 audit_rule;
> > > > +};
> > > > +
> > > > struct fanotify_response {
> > > > __s32 fd;
> > > > __u32 response;
> > > > + __u32 extra_info_type;
> > > > + union fanotify_response_extra extra_info;
> > >
> > > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
> >
> > Again, the intent was to make it fixed for now and change it later if
> > needed, but that was a shortsighted approach...
> >
> > I don't see a need for a len in all response types. _NONE doesn't need
> > any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> > size and layout as needed, including a len field if it is needed.
> >
>
> len is part of a common response info header.
> It is meant to make writing generic code.
> So Jan's email.
Yes. The reason why I want 'type' + 'len' information for every extra
response type is so that the code can be layered properly. Fanotify has no
bussiness in understanding the details of the additional info (or its
expected length) passed from userspace. That is the knowledge that should
stay within the subsystem this info is for. So the length of info record
needs to be passed in the generic info header.
To give an example imagine a situation when we'd like to attach two
different info records to a response, each for a different subsystem. Then
fanotify has to split response buffer and pass each info to the target
subsystem or maybe we'd just pass all info to both subsystems and define
they should ignore info they don't understand but in either case we need to
have a way to be able to separate different info records without apriori
knowledge what they actually mean or what is their expected length.
> > > I don't know if we want to make this flexible enough to allow for multiple
> > > records in the future like we do in events, but the common wisdom of
> > > the universe says that if we don't do it, we will need it.
> >
> > It did occur to me that this could be used for other than audit, hence
> > the renaming of the ..."_NONE" macro.
> >
> > We should be able in the future to define a type that is extensible or
> > has multiple records. We have (2^32) - 2 types left to work with.
> >
>
> The way this was done when we first introduced event info
> records was the same. We only allowed one type of record
> and a single record to begin with, but the format allowed for
> extending to multiple records.
>
> struct fanotify_event_metadata already had event_len and
> metadata_len, so that was convenient. Supporting multi
> records only required that every record has a header with its
> own len.
>
> As far as I can tell, the case of fanotify_response is different
> because we have the count argument of write(), which serves
> as the total response_len.
Yes.
> If we ever want to be able to extend the base fanotify_response,
> add fields to it not as extra info records, then we need to add
> response_metadata_len to struct fanotify_response, but I think that
> would be over design.
Yeah, I don't think that will happen. The standard response metadata is
basically fixed by backward compatibility constraints. If we need to extend
it in the future, I would prefer the extension to be in a form of an extra
info record.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-16 20:22 ` Richard Guy Briggs
@ 2022-05-17 7:16 ` kernel test robot
-1 siblings, 0 replies; 33+ messages in thread
From: kernel test robot @ 2022-05-17 7:16 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-fsdevel
Cc: kbuild-all, Paul Moore, Eric Paris, Steve Grubb,
Richard Guy Briggs, Jan Kara, Amir Goldstein
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: m68k-defconfig (https://download.01.org/0day-ci/archive/20220517/202205171541.x3KcGj83-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> include/linux/kern_levels.h:5:25: warning: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:418:25: note: in definition of macro 'printk_index_wrap'
418 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:132:17: note: in expansion of macro 'printk'
132 | printk(fmt, ##__VA_ARGS__); \
| ^~~~~~
include/linux/printk.h:576:9: note: in expansion of macro 'no_printk'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
include/linux/kern_levels.h:15:25: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:576:19: note: in expansion of macro 'KERN_DEBUG'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro 'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-17 7:16 ` kernel test robot
0 siblings, 0 replies; 33+ messages in thread
From: kernel test robot @ 2022-05-17 7:16 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-fsdevel
Cc: Jan Kara, Richard Guy Briggs, Amir Goldstein, kbuild-all, Eric Paris
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: m68k-defconfig (https://download.01.org/0day-ci/archive/20220517/202205171541.x3KcGj83-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> include/linux/kern_levels.h:5:25: warning: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
5 | #define KERN_SOH "\001" /* ASCII Start Of Header */
| ^~~~~~
include/linux/printk.h:418:25: note: in definition of macro 'printk_index_wrap'
418 | _p_func(_fmt, ##__VA_ARGS__); \
| ^~~~
include/linux/printk.h:132:17: note: in expansion of macro 'printk'
132 | printk(fmt, ##__VA_ARGS__); \
| ^~~~~~
include/linux/printk.h:576:9: note: in expansion of macro 'no_printk'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~
include/linux/kern_levels.h:15:25: note: in expansion of macro 'KERN_SOH'
15 | #define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
| ^~~~~~~~
include/linux/printk.h:576:19: note: in expansion of macro 'KERN_DEBUG'
576 | no_printk(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
| ^~~~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro 'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~
vim +5 include/linux/kern_levels.h
314ba3520e513a Joe Perches 2012-07-30 4
04d2c8c83d0e3a Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c83d0e3a Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c83d0e3a Joe Perches 2012-07-30 7
--
0-DAY CI Kernel Test Service
https://01.org/lkp
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
2022-05-16 20:22 ` Richard Guy Briggs
@ 2022-05-17 7:26 ` kernel test robot
-1 siblings, 0 replies; 33+ messages in thread
From: kernel test robot @ 2022-05-17 7:26 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-fsdevel
Cc: kbuild-all, Paul Moore, Eric Paris, Steve Grubb,
Richard Guy Briggs, Jan Kara, Amir Goldstein
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220517/202205171508.anzweWlm-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> fs/notify/fanotify/fanotify_user.c:325:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:336:21: note: in definition of macro 'pr_fmt'
336 | #define pr_fmt(fmt) fmt
| ^~~
include/linux/dynamic_debug.h:152:9: note: in expansion of macro '__dynamic_func_call'
152 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:162:9: note: in expansion of macro '_dynamic_func_call'
162 | _dynamic_func_call(fmt, __dynamic_pr_debug, \
| ^~~~~~~~~~~~~~~~~~
include/linux/printk.h:570:9: note: in expansion of macro 'dynamic_pr_debug'
570 | dynamic_pr_debug(fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro 'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:65: note: format string is defined here
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ~~^
| |
| long unsigned int
| %u
vim +325 fs/notify/fanotify/fanotify_user.c
316
317 static int process_access_response(struct fsnotify_group *group,
318 struct fanotify_response *response_struct,
319 size_t count)
320 {
321 struct fanotify_perm_event *event;
322 int fd = response_struct->fd;
323 u32 response = response_struct->response;
324
> 325 pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
326 group, fd, response, response_struct->extra_info_type, count);
327 if (fd < 0)
328 return -EINVAL;
329 /*
330 * make sure the response is valid, if invalid we do nothing and either
331 * userspace can send a valid response or we will clean it up after the
332 * timeout
333 */
334 if (FAN_INVALID_RESPONSE_MASK(response))
335 return -EINVAL;
336 if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
337 return -EINVAL;
338 if (response & FAN_EXTRA) {
339 if (count < offsetofend(struct fanotify_response, extra_info_type))
340 return -EINVAL;
341 switch (response_struct->extra_info_type) {
342 case FAN_RESPONSE_INFO_NONE:
343 break;
344 case FAN_RESPONSE_INFO_AUDIT_RULE:
345 if (count < offsetofend(struct fanotify_response, extra_info))
346 return -EINVAL;
347 break;
348 default:
349 return -EINVAL;
350 }
351 }
352 spin_lock(&group->notification_lock);
353 list_for_each_entry(event, &group->fanotify_data.access_list,
354 fae.fse.list) {
355 if (event->fd != fd)
356 continue;
357
358 list_del_init(&event->fae.fse.list);
359 finish_permission_event(group, event, response_struct);
360 wake_up(&group->fanotify_data.access_waitq);
361 return 0;
362 }
363 spin_unlock(&group->notification_lock);
364
365 return -ENOENT;
366 }
367
--
0-DAY CI Kernel Test Service
https://01.org/lkp
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
@ 2022-05-17 7:26 ` kernel test robot
0 siblings, 0 replies; 33+ messages in thread
From: kernel test robot @ 2022-05-17 7:26 UTC (permalink / raw)
To: Richard Guy Briggs, Linux-Audit Mailing List, LKML, linux-fsdevel
Cc: Jan Kara, Richard Guy Briggs, Amir Goldstein, kbuild-all, Eric Paris
Hi Richard,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on jack-fs/fsnotify]
[also build test WARNING on pcmoore-audit/next linux/master linus/master v5.18-rc7 next-20220516]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220517/202205171508.anzweWlm-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220517-044904
git checkout 4d1fc23ae264424a2007ef5a3cfc1b4dbc8d82db
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/notify/fanotify/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/asm-generic/bug.h:22,
from arch/m68k/include/asm/bug.h:32,
from include/linux/bug.h:5,
from include/linux/thread_info.h:13,
from include/asm-generic/preempt.h:5,
from ./arch/m68k/include/generated/asm/preempt.h:1,
from include/linux/preempt.h:78,
from arch/m68k/include/asm/irqflags.h:6,
from include/linux/irqflags.h:16,
from arch/m68k/include/asm/atomic.h:6,
from include/linux/atomic.h:7,
from include/linux/rcupdate.h:25,
from include/linux/sysctl.h:26,
from include/linux/fanotify.h:5,
from fs/notify/fanotify/fanotify_user.c:2:
fs/notify/fanotify/fanotify_user.c: In function 'process_access_response':
>> fs/notify/fanotify/fanotify_user.c:325:18: warning: format '%lu' expects argument of type 'long unsigned int', but argument 8 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/printk.h:336:21: note: in definition of macro 'pr_fmt'
336 | #define pr_fmt(fmt) fmt
| ^~~
include/linux/dynamic_debug.h:152:9: note: in expansion of macro '__dynamic_func_call'
152 | __dynamic_func_call(__UNIQUE_ID(ddebug), fmt, func, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~~~~
include/linux/dynamic_debug.h:162:9: note: in expansion of macro '_dynamic_func_call'
162 | _dynamic_func_call(fmt, __dynamic_pr_debug, \
| ^~~~~~~~~~~~~~~~~~
include/linux/printk.h:570:9: note: in expansion of macro 'dynamic_pr_debug'
570 | dynamic_pr_debug(fmt, ##__VA_ARGS__)
| ^~~~~~~~~~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:9: note: in expansion of macro 'pr_debug'
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ^~~~~~~~
fs/notify/fanotify/fanotify_user.c:325:65: note: format string is defined here
325 | pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
| ~~^
| |
| long unsigned int
| %u
vim +325 fs/notify/fanotify/fanotify_user.c
316
317 static int process_access_response(struct fsnotify_group *group,
318 struct fanotify_response *response_struct,
319 size_t count)
320 {
321 struct fanotify_perm_event *event;
322 int fd = response_struct->fd;
323 u32 response = response_struct->response;
324
> 325 pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
326 group, fd, response, response_struct->extra_info_type, count);
327 if (fd < 0)
328 return -EINVAL;
329 /*
330 * make sure the response is valid, if invalid we do nothing and either
331 * userspace can send a valid response or we will clean it up after the
332 * timeout
333 */
334 if (FAN_INVALID_RESPONSE_MASK(response))
335 return -EINVAL;
336 if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
337 return -EINVAL;
338 if (response & FAN_EXTRA) {
339 if (count < offsetofend(struct fanotify_response, extra_info_type))
340 return -EINVAL;
341 switch (response_struct->extra_info_type) {
342 case FAN_RESPONSE_INFO_NONE:
343 break;
344 case FAN_RESPONSE_INFO_AUDIT_RULE:
345 if (count < offsetofend(struct fanotify_response, extra_info))
346 return -EINVAL;
347 break;
348 default:
349 return -EINVAL;
350 }
351 }
352 spin_lock(&group->notification_lock);
353 list_for_each_entry(event, &group->fanotify_data.access_list,
354 fae.fse.list) {
355 if (event->fd != fd)
356 continue;
357
358 list_del_init(&event->fae.fse.list);
359 finish_permission_event(group, event, response_struct);
360 wake_up(&group->fanotify_data.access_waitq);
361 return 0;
362 }
363 spin_unlock(&group->notification_lock);
364
365 return -ENOENT;
366 }
367
--
0-DAY CI Kernel Test Service
https://01.org/lkp
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
^ permalink raw reply [flat|nested] 33+ messages in thread