* [PATCH v3 0/3] fanotify: Allow user space to pass back additional audit info @ 2022-05-16 20:22 Richard Guy Briggs 2022-05-16 20:22 ` [PATCH v3 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs ` (2 more replies) 0 siblings, 3 replies; 16+ 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 The Fanotify API can be used for access control by requesting permission event notification. The user space tooling that uses it may have a complicated policy that inherently contains additional context for the decision. If this information were available in the audit trail, policy writers can close the loop on debugging policy. Also, if this additional information were available, it would enable the creation of tools that can suggest changes to the policy similar to how audit2allow can help refine labeled security. This patch defines 2 additional fields within the response structure 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 audit system will separate the pieces and log them individually. The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format: type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17 changelog: v1: - first version by Steve Grubb <sgrubb@redhat.com> Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2 v2: - enhancements suggested by Jan Kara <jack@suse.cz> - 1/3 change %d to %u in pr_debug - 2/3 change response from __u32 to __u16 - mod struct fanotify_response and fanotify_perm_event add extra_info_type, extra_info_buf - extra_info_buf size max FANOTIFY_MAX_RESPONSE_EXTRA_LEN, add struct fanotify_response_audit_rule - extend debug statements - remove unneeded macros - [internal] change interface to finish_permission_event() and process_access_response() - 3/3 update format of extra information - [internal] change interface to audit_fanotify() - change ctx_type= to fan_type= Link: https://lore.kernel.org/r/cover.1651174324.git.rgb@redhat.com v3: - 1/3 switch {,__}audit_fanotify() from uint to u32 - 2/3 re-add fanotify_get_response switch case FAN_DENY: to avoid unnecessary churn - add FAN_EXTRA flag to indicate more info and break with old kernel - change response from u16 to u32 to avoid endian issues - change extra_info_buf to union - move low-cost fd check earlier - change FAN_RESPONSE_INFO_AUDIT_NONE to FAN_RESPONSE_INFO_NONE - switch to u32 for internal and __u32 for uapi Link: https://lore.kernel.org/r/cover.1652724390.git.rgb@redhat.com Richard Guy Briggs (3): fanotify: Ensure consistent variable type for response fanotify: define struct members to hold response decision context fanotify: Allow audit to use the full permission event response fs/notify/fanotify/fanotify.c | 6 ++- fs/notify/fanotify/fanotify.h | 4 +- fs/notify/fanotify/fanotify_user.c | 76 +++++++++++++++++++----------- include/linux/audit.h | 9 ++-- include/linux/fanotify.h | 3 ++ include/uapi/linux/fanotify.h | 22 ++++++++- kernel/auditsc.c | 18 +++++-- 7 files changed, 100 insertions(+), 38 deletions(-) -- 2.27.0 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 1/3] fanotify: Ensure consistent variable type for response 2022-05-16 20:22 [PATCH v3 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs @ 2022-05-16 20:22 ` Richard Guy Briggs 2022-05-16 23:06 ` Paul Moore 2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs 2022-05-16 20:22 ` [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response Richard Guy Briggs 2 siblings, 1 reply; 16+ 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 The user space API for the response variable is __u32. This patch makes sure that the whole path through the kernel uses u32 so that there is no sign extension or truncation of the user space response. Suggested-by: Steve Grubb <sgrubb@redhat.com> Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2 Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- fs/notify/fanotify/fanotify.h | 2 +- fs/notify/fanotify/fanotify_user.c | 6 +++--- include/linux/audit.h | 6 +++--- kernel/auditsc.c | 2 +- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index a3d5b751cac5..d66668e06bee 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -425,7 +425,7 @@ FANOTIFY_PE(struct fanotify_event *event) struct fanotify_perm_event { struct fanotify_event fae; struct path path; - unsigned short response; /* userspace answer to the event */ + u32 response; /* userspace answer to the event */ unsigned short state; /* state of the event */ int fd; /* fd we passed to userspace for this event */ }; diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 9b32b76a9c30..721e777ea90b 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -289,7 +289,7 @@ static int create_fd(struct fsnotify_group *group, struct path *path, */ static void finish_permission_event(struct fsnotify_group *group, struct fanotify_perm_event *event, - unsigned int response) + u32 response) __releases(&group->notification_lock) { bool destroy = false; @@ -310,9 +310,9 @@ static int process_access_response(struct fsnotify_group *group, { struct fanotify_perm_event *event; int fd = response_struct->fd; - int response = response_struct->response; + u32 response = response_struct->response; - pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group, + pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group, fd, response); /* * make sure the response is valid, if invalid we do nothing and either diff --git a/include/linux/audit.h b/include/linux/audit.h index d06134ac6245..217784d602b3 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -419,7 +419,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old); extern void __audit_mmap_fd(int fd, int flags); extern void __audit_openat2_how(struct open_how *how); extern void __audit_log_kern_module(char *name); -extern void __audit_fanotify(unsigned int response); +extern void __audit_fanotify(u32 response); extern void __audit_tk_injoffset(struct timespec64 offset); extern void __audit_ntp_log(const struct audit_ntp_data *ad); extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, @@ -526,7 +526,7 @@ static inline void audit_log_kern_module(char *name) __audit_log_kern_module(name); } -static inline void audit_fanotify(unsigned int response) +static inline void audit_fanotify(u32 response) { if (!audit_dummy_context()) __audit_fanotify(response); @@ -686,7 +686,7 @@ static inline void audit_log_kern_module(char *name) { } -static inline void audit_fanotify(unsigned int response) +static inline void audit_fanotify(u32 response) { } static inline void audit_tk_injoffset(struct timespec64 offset) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index ea2ee1181921..6973be0bf6c9 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2893,7 +2893,7 @@ void __audit_log_kern_module(char *name) context->type = AUDIT_KERN_MODULE; } -void __audit_fanotify(unsigned int response) +void __audit_fanotify(u32 response) { audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, "resp=%u", response); -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/3] fanotify: Ensure consistent variable type for response 2022-05-16 20:22 ` [PATCH v3 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs @ 2022-05-16 23:06 ` Paul Moore 0 siblings, 0 replies; 16+ messages in thread From: Paul Moore @ 2022-05-16 23:06 UTC (permalink / raw) To: Richard Guy Briggs Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, Eric Paris, Steve Grubb, Jan Kara, Amir Goldstein On Mon, May 16, 2022 at 4:22 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > The user space API for the response variable is __u32. This patch makes > sure that the whole path through the kernel uses u32 so that there is > no sign extension or truncation of the user space response. > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2 > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > fs/notify/fanotify/fanotify.h | 2 +- > fs/notify/fanotify/fanotify_user.c | 6 +++--- > include/linux/audit.h | 6 +++--- > kernel/auditsc.c | 2 +- > 4 files changed, 8 insertions(+), 8 deletions(-) We're at -rc7, so this should wait until after the upcoming merge window, but it looks okay to me. Acked-by: Paul Moore <paul@paul-moore.com> -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v3 2/3] fanotify: define struct members to hold response decision context 2022-05-16 20:22 [PATCH v3 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs 2022-05-16 20:22 ` [PATCH v3 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs @ 2022-05-16 20:22 ` Richard Guy Briggs 2022-05-17 5:37 ` Amir Goldstein ` (2 more replies) 2022-05-16 20:22 ` [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response Richard Guy Briggs 2 siblings, 3 replies; 16+ 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] 16+ messages in thread
* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context 2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs @ 2022-05-17 5:37 ` Amir Goldstein 2022-05-17 10:32 ` Jan Kara 2022-05-19 0:07 ` Richard Guy Briggs 2022-05-17 7:16 ` kernel test robot 2022-05-17 7:26 ` kernel test robot 2 siblings, 2 replies; 16+ 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] 16+ 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 2022-05-17 11:31 ` Amir Goldstein 2022-05-19 0:07 ` Richard Guy Briggs 1 sibling, 1 reply; 16+ 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] 16+ 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 2022-05-17 12:06 ` Amir Goldstein 0 siblings, 1 reply; 16+ 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] 16+ 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 0 siblings, 0 replies; 16+ 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] 16+ 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 @ 2022-05-19 0:07 ` Richard Guy Briggs 2022-05-19 6:03 ` Amir Goldstein 1 sibling, 1 reply; 16+ 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] 16+ 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 2022-05-19 9:55 ` Jan Kara 0 siblings, 1 reply; 16+ 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] 16+ 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 0 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context 2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs 2022-05-17 5:37 ` Amir Goldstein @ 2022-05-17 7:16 ` kernel test robot 2022-05-17 7:26 ` kernel test robot 2 siblings, 0 replies; 16+ 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] 16+ messages in thread
* Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context 2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs 2022-05-17 5:37 ` Amir Goldstein 2022-05-17 7:16 ` kernel test robot @ 2022-05-17 7:26 ` kernel test robot 2 siblings, 0 replies; 16+ 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] 16+ messages in thread
* [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response 2022-05-16 20:22 [PATCH v3 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs 2022-05-16 20:22 ` [PATCH v3 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs 2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs @ 2022-05-16 20:22 ` Richard Guy Briggs 2022-05-17 1:42 ` Paul Moore 2 siblings, 1 reply; 16+ 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 passes the full value so that the audit function can use all of it. The audit function was updated to log the additional information in the AUDIT_FANOTIFY record. The following is an example of the new record format: type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17 Suggested-by: Steve Grubb <sgrubb@redhat.com> Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- fs/notify/fanotify/fanotify.c | 4 +++- include/linux/audit.h | 9 +++++---- kernel/auditsc.c | 18 +++++++++++++++--- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index ea0e60488f12..85ce36e59e0c 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -273,7 +273,9 @@ static int fanotify_get_response(struct fsnotify_group *group, /* Check if the response should be audited */ if (event->response & FAN_AUDIT) - audit_fanotify(event->response & ~FAN_AUDIT); + audit_fanotify(event->response & ~FAN_AUDIT, + event->extra_info_type, + &event->extra_info); pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, group, event, ret); diff --git a/include/linux/audit.h b/include/linux/audit.h index 217784d602b3..737f1c109aa1 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -14,6 +14,7 @@ #include <linux/audit_arch.h> #include <uapi/linux/audit.h> #include <uapi/linux/netfilter/nf_tables.h> +#include <uapi/linux/fanotify.h> #define AUDIT_INO_UNSET ((unsigned long)-1) #define AUDIT_DEV_UNSET ((dev_t)-1) @@ -419,7 +420,7 @@ extern void __audit_log_capset(const struct cred *new, const struct cred *old); extern void __audit_mmap_fd(int fd, int flags); extern void __audit_openat2_how(struct open_how *how); extern void __audit_log_kern_module(char *name); -extern void __audit_fanotify(u32 response); +extern void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info); extern void __audit_tk_injoffset(struct timespec64 offset); extern void __audit_ntp_log(const struct audit_ntp_data *ad); extern void __audit_log_nfcfg(const char *name, u8 af, unsigned int nentries, @@ -526,10 +527,10 @@ static inline void audit_log_kern_module(char *name) __audit_log_kern_module(name); } -static inline void audit_fanotify(u32 response) +static inline void audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info) { if (!audit_dummy_context()) - __audit_fanotify(response); + __audit_fanotify(response, type, info); } static inline void audit_tk_injoffset(struct timespec64 offset) @@ -686,7 +687,7 @@ static inline void audit_log_kern_module(char *name) { } -static inline void audit_fanotify(u32 response) +static inline void audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info) { } static inline void audit_tk_injoffset(struct timespec64 offset) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 6973be0bf6c9..cb93c6ed07cd 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -64,6 +64,7 @@ #include <uapi/linux/limits.h> #include <uapi/linux/netfilter/nf_tables.h> #include <uapi/linux/openat2.h> // struct open_how +#include <uapi/linux/fanotify.h> #include "audit.h" @@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name) context->type = AUDIT_KERN_MODULE; } -void __audit_fanotify(u32 response) +void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info) { - audit_log(audit_context(), GFP_KERNEL, - AUDIT_FANOTIFY, "resp=%u", response); + switch (type) { + case FAN_RESPONSE_INFO_AUDIT_RULE: + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=%u fan_ctx=%u", + response, type, info->audit_rule); + break; + case FAN_RESPONSE_INFO_NONE: + default: + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=%u fan_ctx=?", + response, type); + break; + } } void __audit_tk_injoffset(struct timespec64 offset) -- 2.27.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response 2022-05-16 20:22 ` [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response Richard Guy Briggs @ 2022-05-17 1:42 ` Paul Moore 2022-05-17 1:57 ` Richard Guy Briggs 0 siblings, 1 reply; 16+ messages in thread From: Paul Moore @ 2022-05-17 1:42 UTC (permalink / raw) To: Richard Guy Briggs Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, Eric Paris, Steve Grubb, Jan Kara, Amir Goldstein On Mon, May 16, 2022 at 4:22 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > This patch passes the full value so that the audit function can use all > of it. The audit function was updated to log the additional information in > the AUDIT_FANOTIFY record. The following is an example of the new record > format: > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17 > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > fs/notify/fanotify/fanotify.c | 4 +++- > include/linux/audit.h | 9 +++++---- > kernel/auditsc.c | 18 +++++++++++++++--- > 3 files changed, 23 insertions(+), 8 deletions(-) ... > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 6973be0bf6c9..cb93c6ed07cd 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > -void __audit_fanotify(u32 response) > +void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info) > { > - audit_log(audit_context(), GFP_KERNEL, > - AUDIT_FANOTIFY, "resp=%u", response); > + switch (type) { > + case FAN_RESPONSE_INFO_AUDIT_RULE: > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_ctx=%u", > + response, type, info->audit_rule); > + break; > + case FAN_RESPONSE_INFO_NONE: > + default: > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_ctx=?", > + response, type); > + break; > + } > } Two things: * Instead of "fan_ctx=", would it make sense to call it "fan_extra=" to better match the UAPI struct? I don't feel strongly either way, but it did occur to me just now while looking at the code so I thought I would mention it. * I'm also wondering if there is a way to be a bit proactive about future proofing this field. Since we already hex encode some fields with "bad" characters, would it make sense to hex encode this field too? Not for the "bad" character reason, but more as a way of marshalling the fanotify_response_extra union into an audit record. I can't see far enough into the future to know if this would be a good idea or not, but like the other point above, it popped into my head while looking at the code so I thought I would put it in the email :) -- paul-moore.com ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response 2022-05-17 1:42 ` Paul Moore @ 2022-05-17 1:57 ` Richard Guy Briggs 0 siblings, 0 replies; 16+ messages in thread From: Richard Guy Briggs @ 2022-05-17 1:57 UTC (permalink / raw) To: Paul Moore Cc: Linux-Audit Mailing List, LKML, linux-fsdevel, Eric Paris, Steve Grubb, Jan Kara, Amir Goldstein On 2022-05-16 21:42, Paul Moore wrote: > On Mon, May 16, 2022 at 4:22 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > This patch passes the full value so that the audit function can use all > > of it. The audit function was updated to log the additional information in > > the AUDIT_FANOTIFY record. The following is an example of the new record > > format: > > > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_ctx=17 > > > > Suggested-by: Steve Grubb <sgrubb@redhat.com> > > Link: https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > fs/notify/fanotify/fanotify.c | 4 +++- > > include/linux/audit.h | 9 +++++---- > > kernel/auditsc.c | 18 +++++++++++++++--- > > 3 files changed, 23 insertions(+), 8 deletions(-) > > ... > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 6973be0bf6c9..cb93c6ed07cd 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2893,10 +2894,21 @@ void __audit_log_kern_module(char *name) > > context->type = AUDIT_KERN_MODULE; > > } > > > > -void __audit_fanotify(u32 response) > > +void __audit_fanotify(u32 response, u32 type, union fanotify_response_extra *info) > > { > > - audit_log(audit_context(), GFP_KERNEL, > > - AUDIT_FANOTIFY, "resp=%u", response); > > + switch (type) { > > + case FAN_RESPONSE_INFO_AUDIT_RULE: > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_ctx=%u", > > + response, type, info->audit_rule); > > + break; > > + case FAN_RESPONSE_INFO_NONE: > > + default: > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_ctx=?", > > + response, type); > > + break; > > + } > > } > > Two things: > > * Instead of "fan_ctx=", would it make sense to call it "fan_extra=" > to better match the UAPI struct? I don't feel strongly either way, > but it did occur to me just now while looking at the code so I thought > I would mention it. Yes, this is a good point. This is the reason I changed from FAN_RESPONSE_INFO_AUDIT_NONE to FAN_RESPONSE_INFO_NONE, anticipating that the extra information could have nothing to do with audit. > * I'm also wondering if there is a way to be a bit proactive about > future proofing this field. Since we already hex encode some fields > with "bad" characters, would it make sense to hex encode this field > too? Not for the "bad" character reason, but more as a way of > marshalling the fanotify_response_extra union into an audit record. I > can't see far enough into the future to know if this would be a good > idea or not, but like the other point above, it popped into my head > while looking at the code so I thought I would put it in the email :) I resisted that idea because it adds overhead and makes it more complex than currently necessary. I'm open to it, but would like to hear Steve's input on this. Thanks for the quick response. > paul-moore.com - 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] 16+ messages in thread
end of thread, other threads:[~2022-05-19 9:55 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-16 20:22 [PATCH v3 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs 2022-05-16 20:22 ` [PATCH v3 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs 2022-05-16 23:06 ` Paul Moore 2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs 2022-05-17 5:37 ` Amir Goldstein 2022-05-17 10:32 ` Jan Kara 2022-05-17 11:31 ` Amir Goldstein 2022-05-17 12:06 ` Amir Goldstein 2022-05-19 0:07 ` Richard Guy Briggs 2022-05-19 6:03 ` Amir Goldstein 2022-05-19 9:55 ` Jan Kara 2022-05-17 7:16 ` kernel test robot 2022-05-17 7:26 ` kernel test robot 2022-05-16 20:22 ` [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response Richard Guy Briggs 2022-05-17 1:42 ` Paul Moore 2022-05-17 1:57 ` Richard Guy Briggs
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).