| Message ID | c4ae9b882c07ea9cac64094294da5edc0756bb50.1659996830.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: Allow user space to pass back additional audit info | expand |
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 linus/master v5.19 next-20220810]
[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#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Richard-Guy-Briggs/fanotify-Allow-user-space-to-pass-back-additional-audit-info/20220810-012825
base: https://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git fsnotify
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220811/202208110406.Lb3ONrcP-lkp@intel.com/config)
compiler: clang version 16.0.0 (https://github.com/llvm/llvm-project 5f1c7e2cc5a3c07cbc2412e851a7283c1841f520)
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/bee8cac0b7796a753948c83b403a152f8c6acb8c
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/20220810-012825
git checkout bee8cac0b7796a753948c83b403a152f8c6acb8c
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> kernel/auditsc.c:2907:8: warning: variable 'ib' set but not used [-Wunused-but-set-variable]
char *ib = buf;
^
1 warning generated.
vim +/ib +2907 kernel/auditsc.c
2902
2903 void __audit_fanotify(u32 response, size_t len, char *buf)
2904 {
2905 struct fanotify_response_info_audit_rule *friar;
2906 size_t c = len;
> 2907 char *ib = buf;
2908
2909 if (!(len && buf)) {
2910 audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
2911 "resp=%u fan_type=0 fan_info=?", response);
2912 return;
2913 }
2914 while (c >= sizeof(struct fanotify_response_info_header)) {
2915 friar = (struct fanotify_response_info_audit_rule *)buf;
2916 switch (friar->hdr.type) {
2917 case FAN_RESPONSE_INFO_AUDIT_RULE:
2918 if (friar->hdr.len < sizeof(*friar)) {
2919 audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
2920 "resp=%u fan_type=%u fan_info=(incomplete)",
2921 response, friar->hdr.type);
2922 return;
2923 }
2924 audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
2925 "resp=%u fan_type=%u fan_info=%u",
2926 response, friar->hdr.type, friar->audit_rule);
2927 }
2928 c -= friar->hdr.len;
2929 ib += friar->hdr.len;
2930 }
2931 }
2932
On Tue, Aug 9, 2022 at 1:23 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_info=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 | 3 ++- > include/linux/audit.h | 9 +++++---- > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++--- > 3 files changed, 35 insertions(+), 8 deletions(-) You've hopefully already seen the kernel test robot build warning, so I won't bring that up again, but a few comments below ... > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 0f36062521f4..36c3ed1af085 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -276,7 +276,8 @@ 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->info_len, event->info_buf); > > 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 3ea198a2cd59..c69efdba12ca 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) > @@ -417,7 +418,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, size_t len, char *buf); > 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, > @@ -524,10 +525,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, size_t len, char *buf) > { > if (!audit_dummy_context()) > - __audit_fanotify(response); > + __audit_fanotify(response, len, buf); > } > > static inline void audit_tk_injoffset(struct timespec64 offset) > @@ -684,7 +685,7 @@ static inline void audit_log_kern_module(char *name) > { > } > > -static inline void audit_fanotify(u32 response) > +static inline void audit_fanotify(u32 response, size_t len, char *buf) > { } > > static inline void audit_tk_injoffset(struct timespec64 offset) > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index 433418d73584..f000fec52360 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" > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > -void __audit_fanotify(u32 response) > +void __audit_fanotify(u32 response, size_t len, char *buf) > { > - audit_log(audit_context(), GFP_KERNEL, > - AUDIT_FANOTIFY, "resp=%u", response); > + struct fanotify_response_info_audit_rule *friar; > + size_t c = len; > + char *ib = buf; > + > + if (!(len && buf)) { > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=0 fan_info=?", response); > + return; > + } > + while (c >= sizeof(struct fanotify_response_info_header)) { > + friar = (struct fanotify_response_info_audit_rule *)buf; Since the only use of this at the moment is the fanotify_response_info_rule, why not pass the fanotify_response_info_rule struct directly into this function? We can always change it if we need to in the future without affecting userspace, and it would simplify the code. > + switch (friar->hdr.type) { > + case FAN_RESPONSE_INFO_AUDIT_RULE: > + if (friar->hdr.len < sizeof(*friar)) { > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_info=(incomplete)", > + response, friar->hdr.type); > + return; > + } > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_info=%u", > + response, friar->hdr.type, friar->audit_rule); > + } > + c -= friar->hdr.len; > + ib += friar->hdr.len; > + } > } > > void __audit_tk_injoffset(struct timespec64 offset) > -- > 2.27.0
On 2022-08-15 20:22, Paul Moore wrote: > On Tue, Aug 9, 2022 at 1:23 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_info=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 | 3 ++- > > include/linux/audit.h | 9 +++++---- > > kernel/auditsc.c | 31 ++++++++++++++++++++++++++++--- > > 3 files changed, 35 insertions(+), 8 deletions(-) > > You've hopefully already seen the kernel test robot build warning, so > I won't bring that up again, but a few comments below ... Yes, dealt with... ... > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index 433418d73584..f000fec52360 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" > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > context->type = AUDIT_KERN_MODULE; > > } > > > > -void __audit_fanotify(u32 response) > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > { > > - audit_log(audit_context(), GFP_KERNEL, > > - AUDIT_FANOTIFY, "resp=%u", response); > > + struct fanotify_response_info_audit_rule *friar; > > + size_t c = len; > > + char *ib = buf; > > + > > + if (!(len && buf)) { > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=0 fan_info=?", response); > > + return; > > + } > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > + friar = (struct fanotify_response_info_audit_rule *)buf; > > Since the only use of this at the moment is the > fanotify_response_info_rule, why not pass the > fanotify_response_info_rule struct directly into this function? We > can always change it if we need to in the future without affecting > userspace, and it would simplify the code. Steve, would it make any sense for there to be more than one FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more than one rule that contributes to a notify reason? If not, would it be reasonable to return -EINVAL if there is more than one? > > + switch (friar->hdr.type) { > > + case FAN_RESPONSE_INFO_AUDIT_RULE: > > + if (friar->hdr.len < sizeof(*friar)) { > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_info=(incomplete)", > > + response, friar->hdr.type); > > + return; > > + } > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_info=%u", > > + response, friar->hdr.type, friar->audit_rule); > > + } > > + c -= friar->hdr.len; > > + ib += friar->hdr.len; > > + } > > } > > > > void __audit_tk_injoffset(struct timespec64 offset) > > 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
On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index 433418d73584..f000fec52360 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" > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > context->type = AUDIT_KERN_MODULE; > > > } > > > > > > -void __audit_fanotify(u32 response) > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > { > > > - audit_log(audit_context(), GFP_KERNEL, > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > + struct fanotify_response_info_audit_rule *friar; > > > + size_t c = len; > > > + char *ib = buf; > > > + > > > + if (!(len && buf)) { > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > > + "resp=%u fan_type=0 fan_info=?", response); > > > + return; > > > + } > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > + friar = (struct fanotify_response_info_audit_rule > > > *)buf; > > > > Since the only use of this at the moment is the > > fanotify_response_info_rule, why not pass the > > fanotify_response_info_rule struct directly into this function? We > > can always change it if we need to in the future without affecting > > userspace, and it would simplify the code. > > Steve, would it make any sense for there to be more than one > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > than one rule that contributes to a notify reason? If not, would it be > reasonable to return -EINVAL if there is more than one? I don't see a reason for sending more than one header. What is more probable is the need to send additional data in that header. I was thinking of maybe bit mapping it in the rule number. But I'd suggest padding the struct just in case it needs expanding some day. -Steev
On 2022-08-31 17:25, Steve Grubb wrote: > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > index 433418d73584..f000fec52360 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" > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > > context->type = AUDIT_KERN_MODULE; > > > > } > > > > > > > > -void __audit_fanotify(u32 response) > > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > > { > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > + struct fanotify_response_info_audit_rule *friar; > > > > + size_t c = len; > > > > + char *ib = buf; > > > > + > > > > + if (!(len && buf)) { > > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > > > + "resp=%u fan_type=0 fan_info=?", response); > > > > + return; > > > > + } > > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > > + friar = (struct fanotify_response_info_audit_rule > > > > *)buf; > > > > > > Since the only use of this at the moment is the > > > fanotify_response_info_rule, why not pass the > > > fanotify_response_info_rule struct directly into this function? We > > > can always change it if we need to in the future without affecting > > > userspace, and it would simplify the code. > > > > Steve, would it make any sense for there to be more than one > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > > than one rule that contributes to a notify reason? If not, would it be > > reasonable to return -EINVAL if there is more than one? > > I don't see a reason for sending more than one header. What is more probable > is the need to send additional data in that header. I was thinking of maybe > bit mapping it in the rule number. But I'd suggest padding the struct just in > case it needs expanding some day. This doesn't exactly answer my question about multiple rules contributing to one decision. The need for more as yet undefined information sounds like a good reason to define a new header if that happens. At this point, is it reasonable to throw an error if more than one RULE header appears in a message? The way I had coded this last patchset was to allow for more than one RULE header and each one would get its own record in the event. How many rules total are likely to exist? > -Steev - 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
On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote: > On 2022-08-31 17:25, Steve Grubb wrote: > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > index 433418d73584..f000fec52360 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" > > > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > > > context->type = AUDIT_KERN_MODULE; > > > > > } > > > > > > > > > > -void __audit_fanotify(u32 response) > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > > > { > > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > > + struct fanotify_response_info_audit_rule *friar; > > > > > + size_t c = len; > > > > > + char *ib = buf; > > > > > + > > > > > + if (!(len && buf)) { > > > > > + audit_log(audit_context(), GFP_KERNEL, > > > > > AUDIT_FANOTIFY, > > > > > + "resp=%u fan_type=0 fan_info=?", > > > > > response); > > > > > + return; > > > > > + } > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > > > + friar = (struct fanotify_response_info_audit_rule > > > > > *)buf; > > > > > > > > Since the only use of this at the moment is the > > > > fanotify_response_info_rule, why not pass the > > > > fanotify_response_info_rule struct directly into this function? We > > > > can always change it if we need to in the future without affecting > > > > userspace, and it would simplify the code. > > > > > > Steve, would it make any sense for there to be more than one > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > > > than one rule that contributes to a notify reason? If not, would it be > > > reasonable to return -EINVAL if there is more than one? > > > > I don't see a reason for sending more than one header. What is more > > probable is the need to send additional data in that header. I was > > thinking of maybe bit mapping it in the rule number. But I'd suggest > > padding the struct just in case it needs expanding some day. > > This doesn't exactly answer my question about multiple rules > contributing to one decision. I don't forsee that. > The need for more as yet undefined information sounds like a good reason > to define a new header if that happens. It's much better to pad the struct so that the size doesn't change. > At this point, is it reasonable to throw an error if more than one RULE > header appears in a message? It is a write syscall. I'd silently discard everything else and document that in the man pages. But the fanotify maintainers should really weigh in on this. > The way I had coded this last patchset was to allow for more than one RULE > header and each one would get its own record in the event. I do not forsee a need for this. > How many rules total are likely to exist? Could be a thousand. But I already know some missing information we'd like to return to user space in an audit event, so the bit mapping on the rule number might happen. I'd suggest padding one u32 for future use. -Steve
On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <sgrubb@redhat.com> wrote: > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote: > > On 2022-08-31 17:25, Steve Grubb wrote: > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > > index 433418d73584..f000fec52360 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" > > > > > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > > > > context->type = AUDIT_KERN_MODULE; > > > > > > } > > > > > > > > > > > > -void __audit_fanotify(u32 response) > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > > > > { > > > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > > > + struct fanotify_response_info_audit_rule *friar; > > > > > > + size_t c = len; > > > > > > + char *ib = buf; > > > > > > + > > > > > > + if (!(len && buf)) { > > > > > > + audit_log(audit_context(), GFP_KERNEL, > > > > > > AUDIT_FANOTIFY, > > > > > > + "resp=%u fan_type=0 fan_info=?", > > > > > > response); > > > > > > + return; > > > > > > + } > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > > > > + friar = (struct fanotify_response_info_audit_rule > > > > > > *)buf; > > > > > > > > > > Since the only use of this at the moment is the > > > > > fanotify_response_info_rule, why not pass the > > > > > fanotify_response_info_rule struct directly into this function? We > > > > > can always change it if we need to in the future without affecting > > > > > userspace, and it would simplify the code. > > > > > > > > Steve, would it make any sense for there to be more than one > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > > > > than one rule that contributes to a notify reason? If not, would it be > > > > reasonable to return -EINVAL if there is more than one? > > > > > > I don't see a reason for sending more than one header. What is more > > > probable is the need to send additional data in that header. I was > > > thinking of maybe bit mapping it in the rule number. But I'd suggest > > > padding the struct just in case it needs expanding some day. > > > > This doesn't exactly answer my question about multiple rules > > contributing to one decision. > > I don't forsee that. > > > The need for more as yet undefined information sounds like a good reason > > to define a new header if that happens. > > It's much better to pad the struct so that the size doesn't change. > > > At this point, is it reasonable to throw an error if more than one RULE > > header appears in a message? > > It is a write syscall. I'd silently discard everything else and document that > in the man pages. But the fanotify maintainers should really weigh in on > this. > > > The way I had coded this last patchset was to allow for more than one RULE > > header and each one would get its own record in the event. > > I do not forsee a need for this. > > > How many rules total are likely to exist? > > Could be a thousand. But I already know some missing information we'd like to > return to user space in an audit event, so the bit mapping on the rule number > might happen. I'd suggest padding one u32 for future use. A better way to handle an expansion like that would be to have a length/version field at the top of the struct that could be used to determine the size and layout of the struct. However, to be clear, my original suggestion of passing the fanotify_response_info_rule struct internally didn't require any additional future proofing as it is an internal implementation detail and not something that is exposed to userspace; the function arguments could be changed in the future and not break userspace. I'm not quite sure how we ended up on this topic ...
On Wed 31-08-22 21:47:09, Paul Moore wrote: > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <sgrubb@redhat.com> wrote: > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote: > > > On 2022-08-31 17:25, Steve Grubb wrote: > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > > > index 433418d73584..f000fec52360 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" > > > > > > > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > > > > > context->type = AUDIT_KERN_MODULE; > > > > > > > } > > > > > > > > > > > > > > -void __audit_fanotify(u32 response) > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > > > > > { > > > > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > > > > + struct fanotify_response_info_audit_rule *friar; > > > > > > > + size_t c = len; > > > > > > > + char *ib = buf; > > > > > > > + > > > > > > > + if (!(len && buf)) { > > > > > > > + audit_log(audit_context(), GFP_KERNEL, > > > > > > > AUDIT_FANOTIFY, > > > > > > > + "resp=%u fan_type=0 fan_info=?", > > > > > > > response); > > > > > > > + return; > > > > > > > + } > > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > > > > > + friar = (struct fanotify_response_info_audit_rule > > > > > > > *)buf; > > > > > > > > > > > > Since the only use of this at the moment is the > > > > > > fanotify_response_info_rule, why not pass the > > > > > > fanotify_response_info_rule struct directly into this function? We > > > > > > can always change it if we need to in the future without affecting > > > > > > userspace, and it would simplify the code. > > > > > > > > > > Steve, would it make any sense for there to be more than one > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > > > > > than one rule that contributes to a notify reason? If not, would it be > > > > > reasonable to return -EINVAL if there is more than one? > > > > > > > > I don't see a reason for sending more than one header. What is more > > > > probable is the need to send additional data in that header. I was > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest > > > > padding the struct just in case it needs expanding some day. > > > > > > This doesn't exactly answer my question about multiple rules > > > contributing to one decision. > > > > I don't forsee that. > > > > > The need for more as yet undefined information sounds like a good reason > > > to define a new header if that happens. > > > > It's much better to pad the struct so that the size doesn't change. > > > > > At this point, is it reasonable to throw an error if more than one RULE > > > header appears in a message? > > > > It is a write syscall. I'd silently discard everything else and document that > > in the man pages. But the fanotify maintainers should really weigh in on > > this. > > > > > The way I had coded this last patchset was to allow for more than one RULE > > > header and each one would get its own record in the event. > > > > I do not forsee a need for this. > > > > > How many rules total are likely to exist? > > > > Could be a thousand. But I already know some missing information we'd like to > > return to user space in an audit event, so the bit mapping on the rule number > > might happen. I'd suggest padding one u32 for future use. > > A better way to handle an expansion like that would be to have a > length/version field at the top of the struct that could be used to > determine the size and layout of the struct. We already do have the 'type' and 'len' fields in struct fanotify_response_info_header. So if audit needs to pass more information, we can define a new 'type' and either make it replace the current struct fanotify_response_info_audit_rule or make it expand the information in it. At least this is how we handle similar situation when fanotify wants to report some new bits of information to userspace. That being said if audit wants to have u32 pad in its struct fanotify_response_info_audit_rule for future "optional" expansion I'm not strictly opposed to that but I don't think it is a good idea. It is tricky to safely start using the new field. Audit subsystem can define that the kernel currently just ignores the field. And new kernel could start using the passed information in the additional field but that is somewhat risky because until that moment userspace can be passing random garbage in that unused field and thus break when running on new kernel that tries to make sense of it. Sure you can say it is broken userspace that does not properly initialize the padding field but history has shown us multiple times that events like these do happen and the breakage was unpleasant enough for users that the kernel just had to revert back to ignoring the field. Alternatively the kernel could bail with error if the new field is non-zero but that would block new userspace using that field from running on old kernel. But presumably the new userspace could be handling the error and writing another response with new field zeroed out. That would be a safe option although not very different from defining a new response type. Ultimately I guess I'll leave it upto audit subsystem what it wants to have in its struct fanotify_response_info_audit_rule because for fanotify subsystem, it is just an opaque blob it is passing. Honza
On Thu, Sep 1, 2022 at 3:52 AM Jan Kara <jack@suse.cz> wrote: > On Wed 31-08-22 21:47:09, Paul Moore wrote: > > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote: > > > > On 2022-08-31 17:25, Steve Grubb wrote: > > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > > > > index 433418d73584..f000fec52360 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" > > > > > > > > > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > > > > > > context->type = AUDIT_KERN_MODULE; > > > > > > > > } > > > > > > > > > > > > > > > > -void __audit_fanotify(u32 response) > > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > > > > > > { > > > > > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > > > > > + struct fanotify_response_info_audit_rule *friar; > > > > > > > > + size_t c = len; > > > > > > > > + char *ib = buf; > > > > > > > > + > > > > > > > > + if (!(len && buf)) { > > > > > > > > + audit_log(audit_context(), GFP_KERNEL, > > > > > > > > AUDIT_FANOTIFY, > > > > > > > > + "resp=%u fan_type=0 fan_info=?", > > > > > > > > response); > > > > > > > > + return; > > > > > > > > + } > > > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > > > > > > + friar = (struct fanotify_response_info_audit_rule > > > > > > > > *)buf; > > > > > > > > > > > > > > Since the only use of this at the moment is the > > > > > > > fanotify_response_info_rule, why not pass the > > > > > > > fanotify_response_info_rule struct directly into this function? We > > > > > > > can always change it if we need to in the future without affecting > > > > > > > userspace, and it would simplify the code. > > > > > > > > > > > > Steve, would it make any sense for there to be more than one > > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > > > > > > than one rule that contributes to a notify reason? If not, would it be > > > > > > reasonable to return -EINVAL if there is more than one? > > > > > > > > > > I don't see a reason for sending more than one header. What is more > > > > > probable is the need to send additional data in that header. I was > > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest > > > > > padding the struct just in case it needs expanding some day. > > > > > > > > This doesn't exactly answer my question about multiple rules > > > > contributing to one decision. > > > > > > I don't forsee that. > > > > > > > The need for more as yet undefined information sounds like a good reason > > > > to define a new header if that happens. > > > > > > It's much better to pad the struct so that the size doesn't change. > > > > > > > At this point, is it reasonable to throw an error if more than one RULE > > > > header appears in a message? > > > > > > It is a write syscall. I'd silently discard everything else and document that > > > in the man pages. But the fanotify maintainers should really weigh in on > > > this. > > > > > > > The way I had coded this last patchset was to allow for more than one RULE > > > > header and each one would get its own record in the event. > > > > > > I do not forsee a need for this. > > > > > > > How many rules total are likely to exist? > > > > > > Could be a thousand. But I already know some missing information we'd like to > > > return to user space in an audit event, so the bit mapping on the rule number > > > might happen. I'd suggest padding one u32 for future use. > > > > A better way to handle an expansion like that would be to have a > > length/version field at the top of the struct that could be used to > > determine the size and layout of the struct. > > We already do have the 'type' and 'len' fields in > struct fanotify_response_info_header. So if audit needs to pass more > information, we can define a new 'type' and either make it replace the > current struct fanotify_response_info_audit_rule or make it expand the > information in it. At least this is how we handle similar situation when > fanotify wants to report some new bits of information to userspace. Perfect, I didn't know that was an option from the fanotify side; I agree that's the right approach. > That being said if audit wants to have u32 pad in its struct > fanotify_response_info_audit_rule for future "optional" expansion I'm not > strictly opposed to that but I don't think it is a good idea. Yes, I'm not a fan of padding out this way, especially when we have better options. > Ultimately I guess I'll leave it upto audit subsystem what it wants to have > in its struct fanotify_response_info_audit_rule because for fanotify > subsystem, it is just an opaque blob it is passing. In that case, let's stick with leveraging the type/len fields in the fanotify_response_info_header struct, that should give us all the flexibility we need. Richard and Steve, it sounds like Steve is already aware of additional information that he wants to send via the fanotify_response_info_audit_rule struct, please include that in the next revision of this patchset. I don't want to get this merged and then soon after have to hack in additional info.
On 2022-09-01 14:31, Paul Moore wrote: > On Thu, Sep 1, 2022 at 3:52 AM Jan Kara <jack@suse.cz> wrote: > > On Wed 31-08-22 21:47:09, Paul Moore wrote: > > > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote: > > > > > On 2022-08-31 17:25, Steve Grubb wrote: > > > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote: > > > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > > > > > index 433418d73584..f000fec52360 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" > > > > > > > > > > > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) > > > > > > > > > context->type = AUDIT_KERN_MODULE; > > > > > > > > > } > > > > > > > > > > > > > > > > > > -void __audit_fanotify(u32 response) > > > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf) > > > > > > > > > { > > > > > > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > > > > > > + struct fanotify_response_info_audit_rule *friar; > > > > > > > > > + size_t c = len; > > > > > > > > > + char *ib = buf; > > > > > > > > > + > > > > > > > > > + if (!(len && buf)) { > > > > > > > > > + audit_log(audit_context(), GFP_KERNEL, > > > > > > > > > AUDIT_FANOTIFY, > > > > > > > > > + "resp=%u fan_type=0 fan_info=?", > > > > > > > > > response); > > > > > > > > > + return; > > > > > > > > > + } > > > > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) { > > > > > > > > > + friar = (struct fanotify_response_info_audit_rule > > > > > > > > > *)buf; > > > > > > > > > > > > > > > > Since the only use of this at the moment is the > > > > > > > > fanotify_response_info_rule, why not pass the > > > > > > > > fanotify_response_info_rule struct directly into this function? We > > > > > > > > can always change it if we need to in the future without affecting > > > > > > > > userspace, and it would simplify the code. > > > > > > > > > > > > > > Steve, would it make any sense for there to be more than one > > > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more > > > > > > > than one rule that contributes to a notify reason? If not, would it be > > > > > > > reasonable to return -EINVAL if there is more than one? > > > > > > > > > > > > I don't see a reason for sending more than one header. What is more > > > > > > probable is the need to send additional data in that header. I was > > > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest > > > > > > padding the struct just in case it needs expanding some day. > > > > > > > > > > This doesn't exactly answer my question about multiple rules > > > > > contributing to one decision. > > > > > > > > I don't forsee that. > > > > > > > > > The need for more as yet undefined information sounds like a good reason > > > > > to define a new header if that happens. > > > > > > > > It's much better to pad the struct so that the size doesn't change. > > > > > > > > > At this point, is it reasonable to throw an error if more than one RULE > > > > > header appears in a message? > > > > > > > > It is a write syscall. I'd silently discard everything else and document that > > > > in the man pages. But the fanotify maintainers should really weigh in on > > > > this. > > > > > > > > > The way I had coded this last patchset was to allow for more than one RULE > > > > > header and each one would get its own record in the event. > > > > > > > > I do not forsee a need for this. > > > > > > > > > How many rules total are likely to exist? > > > > > > > > Could be a thousand. But I already know some missing information we'd like to > > > > return to user space in an audit event, so the bit mapping on the rule number > > > > might happen. I'd suggest padding one u32 for future use. > > > > > > A better way to handle an expansion like that would be to have a > > > length/version field at the top of the struct that could be used to > > > determine the size and layout of the struct. > > > > We already do have the 'type' and 'len' fields in > > struct fanotify_response_info_header. So if audit needs to pass more > > information, we can define a new 'type' and either make it replace the > > current struct fanotify_response_info_audit_rule or make it expand the > > information in it. At least this is how we handle similar situation when > > fanotify wants to report some new bits of information to userspace. > > Perfect, I didn't know that was an option from the fanotify side; I > agree that's the right approach. This is what I expected would be the way to manage changing requirements. > > That being said if audit wants to have u32 pad in its struct > > fanotify_response_info_audit_rule for future "optional" expansion I'm not > > strictly opposed to that but I don't think it is a good idea. > > Yes, I'm not a fan of padding out this way, especially when we have > better options. Agreed. > > Ultimately I guess I'll leave it upto audit subsystem what it wants to have > > in its struct fanotify_response_info_audit_rule because for fanotify > > subsystem, it is just an opaque blob it is passing. > > In that case, let's stick with leveraging the type/len fields in the > fanotify_response_info_header struct, that should give us all the > flexibility we need. > > Richard and Steve, it sounds like Steve is already aware of additional > information that he wants to send via the > fanotify_response_info_audit_rule struct, please include that in the > next revision of this patchset. I don't want to get this merged and > then soon after have to hack in additional info. Steve, please define the type and name of this additional field. I'm not particularly enthusiastic of "u32 pad;" > 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
On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > Ultimately I guess I'll leave it upto audit subsystem what it wants to > > > have in its struct fanotify_response_info_audit_rule because for > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > In that case, let's stick with leveraging the type/len fields in the > > fanotify_response_info_header struct, that should give us all the > > flexibility we need. > > > > Richard and Steve, it sounds like Steve is already aware of additional > > information that he wants to send via the > > fanotify_response_info_audit_rule struct, please include that in the > > next revision of this patchset. I don't want to get this merged and > > then soon after have to hack in additional info. > > Steve, please define the type and name of this additional field. Maybe extra_data, app_data, or extra_info. Something generic that can be reused by any application. Default to 0 if not present. Thanks, -Steve
On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <sgrubb@redhat.com> wrote: > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants to > > > > have in its struct fanotify_response_info_audit_rule because for > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > fanotify_response_info_header struct, that should give us all the > > > flexibility we need. > > > > > > Richard and Steve, it sounds like Steve is already aware of additional > > > information that he wants to send via the > > > fanotify_response_info_audit_rule struct, please include that in the > > > next revision of this patchset. I don't want to get this merged and > > > then soon after have to hack in additional info. > > > > Steve, please define the type and name of this additional field. > > Maybe extra_data, app_data, or extra_info. Something generic that can be > reused by any application. Default to 0 if not present. I think the point is being missed ... The idea is to not speculate on additional fields, as discussed we have ways to handle that, the issue was that Steve implied that he already had ideas for "things" he wanted to add. If there are "things" that need to be added, let's do that now, however if there is just speculation that maybe someday we might need to add something else we can leave that until later.
On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <sgrubb@redhat.com> wrote: > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants > > > > > to > > > > > have in its struct fanotify_response_info_audit_rule because for > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > > fanotify_response_info_header struct, that should give us all the > > > > flexibility we need. > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > additional > > > > information that he wants to send via the > > > > fanotify_response_info_audit_rule struct, please include that in the > > > > next revision of this patchset. I don't want to get this merged and > > > > then soon after have to hack in additional info. > > > > > > Steve, please define the type and name of this additional field. > > > > Maybe extra_data, app_data, or extra_info. Something generic that can be > > reused by any application. Default to 0 if not present. > > I think the point is being missed ... The idea is to not speculate on > additional fields, as discussed we have ways to handle that, the issue > was that Steve implied that he already had ideas for "things" he > wanted to add. If there are "things" that need to be added, let's do > that now, however if there is just speculation that maybe someday we > might need to add something else we can leave that until later. This is not speculation. I know what I want to put there. I know you want to pin it down to exactly what it is. However, when this started a couple years back, one of the concerns was that we're building something specific to 1 user of fanotify. And that it would be better for all future users to have a generic facility that everyone could use if they wanted to. That's why I'm suggesting something generic, its so this is not special purpose that doesn't fit any other use case. -Steve
On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <sgrubb@redhat.com> wrote: > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants > > > > > > to > > > > > > have in its struct fanotify_response_info_audit_rule because for > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in the > > > > > fanotify_response_info_header struct, that should give us all the > > > > > flexibility we need. > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > additional > > > > > information that he wants to send via the > > > > > fanotify_response_info_audit_rule struct, please include that in the > > > > > next revision of this patchset. I don't want to get this merged and > > > > > then soon after have to hack in additional info. > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can be > > > reused by any application. Default to 0 if not present. > > > > I think the point is being missed ... The idea is to not speculate on > > additional fields, as discussed we have ways to handle that, the issue > > was that Steve implied that he already had ideas for "things" he > > wanted to add. If there are "things" that need to be added, let's do > > that now, however if there is just speculation that maybe someday we > > might need to add something else we can leave that until later. > > This is not speculation. I know what I want to put there. I know you want to > pin it down to exactly what it is. However, when this started a couple years > back, one of the concerns was that we're building something specific to 1 user > of fanotify. And that it would be better for all future users to have a > generic facility that everyone could use if they wanted to. That's why I'm > suggesting something generic, its so this is not special purpose that doesn't > fit any other use case. Well, we are talking specifically about fanotify in this thread and dealing with data structures that are specific to fanotify. I can understand wanting to future proof things, but based on what we've seen in this thread I think we are all set in this regard. You mention that you know what you want to put in the struct, why not share the details with all of us so we are all on the same page and can have a proper discussion.
On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote: > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <sgrubb@redhat.com> wrote: > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote: > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it > > > > > > > wants > > > > > > > to > > > > > > > have in its struct fanotify_response_info_audit_rule because > > > > > > > for > > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in > > > > > > the > > > > > > fanotify_response_info_header struct, that should give us all the > > > > > > flexibility we need. > > > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > > additional > > > > > > information that he wants to send via the > > > > > > fanotify_response_info_audit_rule struct, please include that in > > > > > > the > > > > > > next revision of this patchset. I don't want to get this merged > > > > > > and > > > > > > then soon after have to hack in additional info. > > > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can > > > > be > > > > reused by any application. Default to 0 if not present. > > > > > > I think the point is being missed ... The idea is to not speculate on > > > additional fields, as discussed we have ways to handle that, the issue > > > was that Steve implied that he already had ideas for "things" he > > > wanted to add. If there are "things" that need to be added, let's do > > > that now, however if there is just speculation that maybe someday we > > > might need to add something else we can leave that until later. > > > > This is not speculation. I know what I want to put there. I know you want > > to pin it down to exactly what it is. However, when this started a > > couple years back, one of the concerns was that we're building something > > specific to 1 user of fanotify. And that it would be better for all > > future users to have a generic facility that everyone could use if they > > wanted to. That's why I'm suggesting something generic, its so this is > > not special purpose that doesn't fit any other use case. > > Well, we are talking specifically about fanotify in this thread and > dealing with data structures that are specific to fanotify. I can > understand wanting to future proof things, but based on what we've > seen in this thread I think we are all set in this regard. I'm trying to abide by what was suggested by the fs-devel folks. I can live with it. But if you want to make something non-generic for all users of fanotify, call the new field "trusted". This would decern when a decision was made because the file was untrusted or access denied for another reason. > You mention that you know what you want to put in the struct, why not > share the details with all of us so we are all on the same page and > can have a proper discussion. Because I want to abide by the original agreement and not impose opinionated requirements that serve no one else. I'd rather have something anyone can use. I want to play nice. -Steve
On 2022-09-08 22:20, Steve Grubb wrote: > On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote: > > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs > wrote: > > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it > > > > > > > > wants > > > > > > > > to > > > > > > > > have in its struct fanotify_response_info_audit_rule because > > > > > > > > for > > > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in > > > > > > > the > > > > > > > fanotify_response_info_header struct, that should give us all the > > > > > > > flexibility we need. > > > > > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > > > additional > > > > > > > information that he wants to send via the > > > > > > > fanotify_response_info_audit_rule struct, please include that in > > > > > > > the > > > > > > > next revision of this patchset. I don't want to get this merged > > > > > > > and > > > > > > > then soon after have to hack in additional info. > > > > > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can > > > > > be > > > > > reused by any application. Default to 0 if not present. > > > > > > > > I think the point is being missed ... The idea is to not speculate on > > > > additional fields, as discussed we have ways to handle that, the issue > > > > was that Steve implied that he already had ideas for "things" he > > > > wanted to add. If there are "things" that need to be added, let's do > > > > that now, however if there is just speculation that maybe someday we > > > > might need to add something else we can leave that until later. > > > > > > This is not speculation. I know what I want to put there. I know you want > > > to pin it down to exactly what it is. However, when this started a > > > couple years back, one of the concerns was that we're building something > > > specific to 1 user of fanotify. And that it would be better for all > > > future users to have a generic facility that everyone could use if they > > > wanted to. That's why I'm suggesting something generic, its so this is > > > not special purpose that doesn't fit any other use case. > > > > Well, we are talking specifically about fanotify in this thread and > > dealing with data structures that are specific to fanotify. I can > > understand wanting to future proof things, but based on what we've > > seen in this thread I think we are all set in this regard. > > I'm trying to abide by what was suggested by the fs-devel folks. I can live > with it. But if you want to make something non-generic for all users of > fanotify, call the new field "trusted". This would decern when a decision was > made because the file was untrusted or access denied for another reason. So, "u32 trusted;" ? How would you like that formatted? "fan_trust={0|1}" > > You mention that you know what you want to put in the struct, why not > > share the details with all of us so we are all on the same page and > > can have a proper discussion. > > Because I want to abide by the original agreement and not impose opinionated > requirements that serve no one else. I'd rather have something anyone can > use. I want to play nice. If someone else wants to use something, why not give them a type of their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape however they like? > -Steve - 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
On Thu, Sep 8, 2022 at 10:41 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2022-09-08 22:20, Steve Grubb wrote: > > On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote: > > > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote: > > > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs > > wrote: > > > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it > > > > > > > > > wants > > > > > > > > > to > > > > > > > > > have in its struct fanotify_response_info_audit_rule because > > > > > > > > > for > > > > > > > > > fanotify subsystem, it is just an opaque blob it is passing. > > > > > > > > > > > > > > > > In that case, let's stick with leveraging the type/len fields in > > > > > > > > the > > > > > > > > fanotify_response_info_header struct, that should give us all the > > > > > > > > flexibility we need. > > > > > > > > > > > > > > > > Richard and Steve, it sounds like Steve is already aware of > > > > > > > > additional > > > > > > > > information that he wants to send via the > > > > > > > > fanotify_response_info_audit_rule struct, please include that in > > > > > > > > the > > > > > > > > next revision of this patchset. I don't want to get this merged > > > > > > > > and > > > > > > > > then soon after have to hack in additional info. > > > > > > > > > > > > > > Steve, please define the type and name of this additional field. > > > > > > > > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can > > > > > > be > > > > > > reused by any application. Default to 0 if not present. > > > > > > > > > > I think the point is being missed ... The idea is to not speculate on > > > > > additional fields, as discussed we have ways to handle that, the issue > > > > > was that Steve implied that he already had ideas for "things" he > > > > > wanted to add. If there are "things" that need to be added, let's do > > > > > that now, however if there is just speculation that maybe someday we > > > > > might need to add something else we can leave that until later. > > > > > > > > This is not speculation. I know what I want to put there. I know you want > > > > to pin it down to exactly what it is. However, when this started a > > > > couple years back, one of the concerns was that we're building something > > > > specific to 1 user of fanotify. And that it would be better for all > > > > future users to have a generic facility that everyone could use if they > > > > wanted to. That's why I'm suggesting something generic, its so this is > > > > not special purpose that doesn't fit any other use case. > > > > > > Well, we are talking specifically about fanotify in this thread and > > > dealing with data structures that are specific to fanotify. I can > > > understand wanting to future proof things, but based on what we've > > > seen in this thread I think we are all set in this regard. > > > > I'm trying to abide by what was suggested by the fs-devel folks. I can live > > with it. But if you want to make something non-generic for all users of > > fanotify, call the new field "trusted". This would decern when a decision was > > made because the file was untrusted or access denied for another reason. > > So, "u32 trusted;" ? How would you like that formatted? > "fan_trust={0|1}" > > > > You mention that you know what you want to put in the struct, why not > > > share the details with all of us so we are all on the same page and > > > can have a proper discussion. > > > > Because I want to abide by the original agreement and not impose opinionated > > requirements that serve no one else. I'd rather have something anyone can > > use. I want to play nice. > > If someone else wants to use something, why not give them a type of > their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape > however they like? Yes, exactly. The struct is very clearly specific to both fanotify and audit, I see no reason why it needs to be made generic for use by other subsystems when other mechanisms exist to support them.
On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote: > > I'm trying to abide by what was suggested by the fs-devel folks. I can > > live with it. But if you want to make something non-generic for all > > users of fanotify, call the new field "trusted". This would decern when > > a decision was made because the file was untrusted or access denied for > > another reason. > > So, "u32 trusted;" ? How would you like that formatted? > "fan_trust={0|1}" So how does this play out if there is another user? Do they want a num= and trust= if not, then the AUDIT_FANOTIFY record will have multiple formats which is not good. I'd rather suggest something generic that can be interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and then followed by info0= info1= the interpretation of those solely depend on fan_type. If the fan_type does not need both, then any interpretation skips what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is for that format. But make this pivot on fan_type and not actual names. > > > You mention that you know what you want to put in the struct, why not > > > share the details with all of us so we are all on the same page and > > > can have a proper discussion. > > > > Because I want to abide by the original agreement and not impose > > opinionated requirements that serve no one else. I'd rather have > > something anyone can use. I want to play nice. > > If someone else wants to use something, why not give them a type of > their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape > however they like? Please, let's keep AUDIT_FANOTIFY normalized but pivot on fan_type. -Steve
Hello Steve! On Fri 09-09-22 00:03:53, Steve Grubb wrote: > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote: > > > I'm trying to abide by what was suggested by the fs-devel folks. I can > > > live with it. But if you want to make something non-generic for all > > > users of fanotify, call the new field "trusted". This would decern when > > > a decision was made because the file was untrusted or access denied for > > > another reason. > > > > So, "u32 trusted;" ? How would you like that formatted? > > "fan_trust={0|1}" > > So how does this play out if there is another user? Do they want a num= and > trust= if not, then the AUDIT_FANOTIFY record will have multiple formats > which is not good. I'd rather suggest something generic that can be > interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and > then followed by info0= info1= the interpretation of those solely depend on > fan_type. If the fan_type does not need both, then any interpretation skips > what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is > for that format. But make this pivot on fan_type and not actual names. So I think there is some misunderstanding so let me maybe spell out in detail how I see things so that we can get on the same page: It was a requirement from me (and probably Amir) that there is a generic way to attach additional info to a response to fanotify permission event. This is achieved by defining: struct fanotify_response_info_header { __u8 type; __u8 pad; __u16 len; }; which is a generic header and kernel can based on 'len' field decide how large the response structure is (to safely copy it from userspace) and based on 'type' field it can decide who should be the recipient of this extra information (or generally what to do with it). So any additional info needs to start with this header. Then there is: struct fanotify_response_info_audit_rule { struct fanotify_response_info_header hdr; __u32 audit_rule; }; which properly starts with the header and hdr.type is expected to be FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit* subsystem's responsibility. Fanotify code will just pass this as an opaque blob to the audit subsystem. So if you know audit subsystem will also need some other field together with 'audit_rule' now is a good time to add it and it doesn't have to be useful for anybody else besides audit. If someone else will need other information passed along with the response, he will append structure with another header with different 'type' field. In principle, there can be multiple structures appended to fanotify response like <hdr> <data> <hdr> <data> ... and fanotify subsystem will just pass them to different receivers based on the type in 'hdr' field. Also if audit needs to pass even more information along with the respose, we can define a new 'type' for it. But the 'type' space is not infinite so I'd prefer this does not happen too often... I hope this clears out things a bit. Honza
On Friday, September 9, 2022 7:09:44 AM EDT Jan Kara wrote: > Hello Steve! > > On Fri 09-09-22 00:03:53, Steve Grubb wrote: > > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote: > > > > I'm trying to abide by what was suggested by the fs-devel folks. I > > > > can > > > > live with it. But if you want to make something non-generic for all > > > > users of fanotify, call the new field "trusted". This would decern > > > > when > > > > a decision was made because the file was untrusted or access denied > > > > for > > > > another reason. > > > > > > So, "u32 trusted;" ? How would you like that formatted? > > > "fan_trust={0|1}" > > > > So how does this play out if there is another user? Do they want a num= > > and trust= if not, then the AUDIT_FANOTIFY record will have multiple > > formats which is not good. I'd rather suggest something generic that can > > be interpreted based on who's attached to fanotify. IOW we have a > > fan_type=0 and then followed by info0= info1= the interpretation of > > those solely depend on fan_type. If the fan_type does not need both, > > then any interpretation skips what it doesn't need. If fan_type=1, then > > it follows what arg0= and arg1= is for that format. But make this pivot > > on fan_type and not actual names. > So I think there is some misunderstanding so let me maybe spell out in > detail how I see things so that we can get on the same page: > > It was a requirement from me (and probably Amir) that there is a generic > way to attach additional info to a response to fanotify permission event. > This is achieved by defining: > > struct fanotify_response_info_header { > __u8 type; > __u8 pad; > __u16 len; > }; > > which is a generic header and kernel can based on 'len' field decide how > large the response structure is (to safely copy it from userspace) and > based on 'type' field it can decide who should be the recipient of this > extra information (or generally what to do with it). So any additional > info needs to start with this header. > > Then there is: > > struct fanotify_response_info_audit_rule { > struct fanotify_response_info_header hdr; > __u32 audit_rule; > }; > > which properly starts with the header and hdr.type is expected to be > FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type > FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit* > subsystem's responsibility. Fanotify code will just pass this as an opaque > blob to the audit subsystem. > > So if you know audit subsystem will also need some other field together > with 'audit_rule' now is a good time to add it and it doesn't have to be > useful for anybody else besides audit. If someone else will need other > information passed along with the response, he will append structure with > another header with different 'type' field. In principle, there can be > multiple structures appended to fanotify response like > > <hdr> <data> <hdr> <data> ... > > and fanotify subsystem will just pass them to different receivers based > on the type in 'hdr' field. > > Also if audit needs to pass even more information along with the respose, > we can define a new 'type' for it. But the 'type' space is not infinite so > I'd prefer this does not happen too often... > > I hope this clears out things a bit. Yes. Thank you. Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, unknown. -Steve
On 2022-09-09 10:22, Steve Grubb wrote: > On Friday, September 9, 2022 7:09:44 AM EDT Jan Kara wrote: > > Hello Steve! > > > > On Fri 09-09-22 00:03:53, Steve Grubb wrote: > > > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote: > > > > > I'm trying to abide by what was suggested by the fs-devel folks. I > > > > > can > > > > > live with it. But if you want to make something non-generic for all > > > > > users of fanotify, call the new field "trusted". This would decern > > > > > when > > > > > a decision was made because the file was untrusted or access denied > > > > > for > > > > > another reason. > > > > > > > > So, "u32 trusted;" ? How would you like that formatted? > > > > "fan_trust={0|1}" > > > > > > So how does this play out if there is another user? Do they want a num= > > > and trust= if not, then the AUDIT_FANOTIFY record will have multiple > > > formats which is not good. I'd rather suggest something generic that can > > > be interpreted based on who's attached to fanotify. IOW we have a > > > fan_type=0 and then followed by info0= info1= the interpretation of > > > those solely depend on fan_type. If the fan_type does not need both, > > > then any interpretation skips what it doesn't need. If fan_type=1, then > > > it follows what arg0= and arg1= is for that format. But make this pivot > > > on fan_type and not actual names. > > So I think there is some misunderstanding so let me maybe spell out in > > detail how I see things so that we can get on the same page: > > > > It was a requirement from me (and probably Amir) that there is a generic > > way to attach additional info to a response to fanotify permission event. > > This is achieved by defining: > > > > struct fanotify_response_info_header { > > __u8 type; > > __u8 pad; > > __u16 len; > > }; > > > > which is a generic header and kernel can based on 'len' field decide how > > large the response structure is (to safely copy it from userspace) and > > based on 'type' field it can decide who should be the recipient of this > > extra information (or generally what to do with it). So any additional > > info needs to start with this header. > > > > Then there is: > > > > struct fanotify_response_info_audit_rule { > > struct fanotify_response_info_header hdr; > > __u32 audit_rule; > > }; > > > > which properly starts with the header and hdr.type is expected to be > > FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type > > FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit* > > subsystem's responsibility. Fanotify code will just pass this as an opaque > > blob to the audit subsystem. > > > > So if you know audit subsystem will also need some other field together > > with 'audit_rule' now is a good time to add it and it doesn't have to be > > useful for anybody else besides audit. If someone else will need other > > information passed along with the response, he will append structure with > > another header with different 'type' field. In principle, there can be > > multiple structures appended to fanotify response like > > > > <hdr> <data> <hdr> <data> ... > > > > and fanotify subsystem will just pass them to different receivers based > > on the type in 'hdr' field. > > > > Also if audit needs to pass even more information along with the respose, > > we can define a new 'type' for it. But the 'type' space is not infinite so > > I'd prefer this does not happen too often... > > > > I hope this clears out things a bit. > > Yes. Thank you. > > Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, > unknown. type? bitfield? My gut would say that "0" should be "unset"/"unknown", but that is counterintuitive to the values represented. Or "trust" with sub-fields "subj" and "obj"? > -Steve - 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
On Friday, September 9, 2022 10:38:46 AM EDT Richard Guy Briggs wrote: > > Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, > > unknown. > > type? bitfield? My gut would say that "0" should be "unset"/"unknown", > but that is counterintuitive to the values represented. > > Or "trust" with sub-fields "subj" and "obj"? No. just make them separate and u32. subj_trust and obj_trust - no sub fields. If we have sub-fields, that probably means bit mapping and that wasn't wanted. -Steve
On 2022-09-09 10:55, Steve Grubb wrote: > On Friday, September 9, 2022 10:38:46 AM EDT Richard Guy Briggs wrote: > > > Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes, > > > unknown. > > > > type? bitfield? My gut would say that "0" should be "unset"/"unknown", > > but that is counterintuitive to the values represented. > > > > Or "trust" with sub-fields "subj" and "obj"? > > No. just make them separate and u32. subj_trust and obj_trust - no sub fields. > If we have sub-fields, that probably means bit mapping and that wasn't wanted. Ack. > -Steve - 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
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 0f36062521f4..36c3ed1af085 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -276,7 +276,8 @@ 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->info_len, event->info_buf); 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 3ea198a2cd59..c69efdba12ca 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) @@ -417,7 +418,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, size_t len, char *buf); 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, @@ -524,10 +525,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, size_t len, char *buf) { if (!audit_dummy_context()) - __audit_fanotify(response); + __audit_fanotify(response, len, buf); } static inline void audit_tk_injoffset(struct timespec64 offset) @@ -684,7 +685,7 @@ static inline void audit_log_kern_module(char *name) { } -static inline void audit_fanotify(u32 response) +static inline void audit_fanotify(u32 response, size_t len, char *buf) { } static inline void audit_tk_injoffset(struct timespec64 offset) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index 433418d73584..f000fec52360 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" @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name) context->type = AUDIT_KERN_MODULE; } -void __audit_fanotify(u32 response) +void __audit_fanotify(u32 response, size_t len, char *buf) { - audit_log(audit_context(), GFP_KERNEL, - AUDIT_FANOTIFY, "resp=%u", response); + struct fanotify_response_info_audit_rule *friar; + size_t c = len; + char *ib = buf; + + if (!(len && buf)) { + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=0 fan_info=?", response); + return; + } + while (c >= sizeof(struct fanotify_response_info_header)) { + friar = (struct fanotify_response_info_audit_rule *)buf; + switch (friar->hdr.type) { + case FAN_RESPONSE_INFO_AUDIT_RULE: + if (friar->hdr.len < sizeof(*friar)) { + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=%u fan_info=(incomplete)", + response, friar->hdr.type); + return; + } + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=%u fan_info=%u", + response, friar->hdr.type, friar->audit_rule); + } + c -= friar->hdr.len; + ib += friar->hdr.len; + } } void __audit_tk_injoffset(struct timespec64 offset)
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_info=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 | 3 ++- include/linux/audit.h | 9 +++++---- kernel/auditsc.c | 31 ++++++++++++++++++++++++++++--- 3 files changed, 35 insertions(+), 8 deletions(-)