| Message ID | 82aba376bfbb9927ab7146e8e2dee8d844a31dc2.1673989212.git.rgb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: Allow user space to pass back additional audit info | expand |
Hello Richard, I built a new kernel and tested this with old and new user space. It is working as advertised. The only thing I'm wondering about is why we have 3F as the default value when no additional info was sent? Would it be better to just make it 0? Btw, the change to %X makes life easier. Thx. -Steve On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote: > This patch passes the full response 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. > > Currently the only type of fanotify info that is defined is an audit > rule number, but convert it to hex encoding to future-proof the field. > Hex encoding suggested by Paul Moore <paul@paul-moore.com>. > > The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown. > > Sample records: > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 > fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY > msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 > obj_trust=2 > > 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 | 16 +++++++++++++--- > 3 files changed, 20 insertions(+), 8 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 24ec1d66d5a8..29bdd99b29fa 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -273,7 +273,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->audit_rule); > > 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 d6b7d0c7ce43..31086a72e32a 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) > @@ -416,7 +417,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, struct > fanotify_response_info_audit_rule *friar); 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, @@ -523,10 +524,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, struct > fanotify_response_info_audit_rule *friar) { > if (!audit_dummy_context()) > - __audit_fanotify(response); > + __audit_fanotify(response, friar); > } > > static inline void audit_tk_injoffset(struct timespec64 offset) > @@ -679,7 +680,7 @@ static inline void audit_log_kern_module(char *name) > { > } > > -static inline void audit_fanotify(u32 response) > +static inline void audit_fanotify(u32 response, struct > fanotify_response_info_audit_rule *friar) { } > > static inline void audit_tk_injoffset(struct timespec64 offset) > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index d1fb821de104..3133c4175c15 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" > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > -void __audit_fanotify(u32 response) > +void __audit_fanotify(u32 response, struct > fanotify_response_info_audit_rule *friar) { > - audit_log(audit_context(), GFP_KERNEL, > - AUDIT_FANOTIFY, "resp=%u", response); > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2", > + response, FAN_RESPONSE_INFO_NONE); > + return; > + } > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u", > + response, friar->hdr.type, friar->rule_number, > + friar->subj_trust, friar->obj_trust); > } > > void __audit_tk_injoffset(struct timespec64 offset)
On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <sgrubb@redhat.com> wrote: > > Hello Richard, > > I built a new kernel and tested this with old and new user space. It is > working as advertised. The only thing I'm wondering about is why we have 3F > as the default value when no additional info was sent? Would it be better to > just make it 0? ... > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote: > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index d1fb821de104..3133c4175c15 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > > context->type = AUDIT_KERN_MODULE; > > } > > > > -void __audit_fanotify(u32 response) > > +void __audit_fanotify(u32 response, struct > > fanotify_response_info_audit_rule *friar) { > > - audit_log(audit_context(), GFP_KERNEL, > > - AUDIT_FANOTIFY, "resp=%u", response); > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 > obj_trust=2", > > + response, FAN_RESPONSE_INFO_NONE); > > + return; > > + } (I'm working under the assumption that the "fan_info=3F" in the record above is what Steve was referring to in his comment.) I vaguely recall Richard commenting on this in the past, although maybe not ... my thought is that the "3F" is simply the hex encoded "?" character in ASCII ('man 7 ascii' is your friend). I suppose the question is what to do in the FAN_RESPONSE_INFO_NONE case. Historically when we had a missing field we would follow the "field=?" pattern, but I don't recall doing that for a field which was potentially hex encoded, is there an existing case where we use "?" for a field that is hex encoded? If so, we can swap out the "3F" for a more obvious "?". However, another option might be to simply output the current AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g. only "resp=%u". This is a little against the usual guidance of "fields should not disappear from a record", but considering that userspace will always need to support the original resp-only format for compatibility reasons this may be an option. -- paul-moore.com
On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > This patch passes the full response 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. > > Currently the only type of fanotify info that is defined is an audit > rule number, but convert it to hex encoding to future-proof the field. > Hex encoding suggested by Paul Moore <paul@paul-moore.com>. > > The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown. > > Sample records: > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5 > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2 > > 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 | 16 +++++++++++++--- > 3 files changed, 20 insertions(+), 8 deletions(-) ... > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index d1fb821de104..3133c4175c15 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > context->type = AUDIT_KERN_MODULE; > } > > -void __audit_fanotify(u32 response) > +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar) > { > - audit_log(audit_context(), GFP_KERNEL, > - AUDIT_FANOTIFY, "resp=%u", response); > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2", > + response, FAN_RESPONSE_INFO_NONE); > + return; > + } > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > + "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u", > + response, friar->hdr.type, friar->rule_number, > + friar->subj_trust, friar->obj_trust); > } The only thing that comes to mind might be to convert the if-return into a switch statement to make it a bit cleaner and easier to patch in the future, but that is soooo far removed from any real concern that I debated even mentioning it. I only bring it up in case the "3F" discussion results in a respin, and even then I'm not going to hold my ACK over something as silly as a if-return vs switch. For clarity, this is what I was thinking: void __audit_fanontify(...) { switch (type) { case FAN_RESPONSE_INFO_NONE: audit_log(...); break; default: audit_log(...); } }
On 2023-01-20 13:52, Paul Moore wrote: > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <sgrubb@redhat.com> wrote: > > Hello Richard, > > > > I built a new kernel and tested this with old and new user space. It is > > working as advertised. The only thing I'm wondering about is why we have 3F > > as the default value when no additional info was sent? Would it be better to > > just make it 0? > > ... > > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote: > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index d1fb821de104..3133c4175c15 100644 > > > --- a/kernel/auditsc.c > > > +++ b/kernel/auditsc.c > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > > > context->type = AUDIT_KERN_MODULE; > > > } > > > > > > -void __audit_fanotify(u32 response) > > > +void __audit_fanotify(u32 response, struct > > > fanotify_response_info_audit_rule *friar) { > > > - audit_log(audit_context(), GFP_KERNEL, > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 > > obj_trust=2", > > > + response, FAN_RESPONSE_INFO_NONE); > > > + return; > > > + } > > (I'm working under the assumption that the "fan_info=3F" in the record > above is what Steve was referring to in his comment.) > > I vaguely recall Richard commenting on this in the past, although > maybe not ... my thought is that the "3F" is simply the hex encoded > "?" character in ASCII ('man 7 ascii' is your friend). I suppose the > question is what to do in the FAN_RESPONSE_INFO_NONE case. > > Historically when we had a missing field we would follow the "field=?" > pattern, but I don't recall doing that for a field which was > potentially hex encoded, is there an existing case where we use "?" > for a field that is hex encoded? If so, we can swap out the "3F" for > a more obvious "?". I was presuming encoding the zero: "30" > However, another option might be to simply output the current > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g. > only "resp=%u". This is a little against the usual guidance of > "fields should not disappear from a record", but considering that > userspace will always need to support the original resp-only format > for compatibility reasons this may be an option. I don't have a strong opinion. > 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 2023-01-20 13:58, Paul Moore wrote: > On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > This patch passes the full response 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. > > > > Currently the only type of fanotify info that is defined is an audit > > rule number, but convert it to hex encoding to future-proof the field. > > Hex encoding suggested by Paul Moore <paul@paul-moore.com>. > > > > The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown. > > > > Sample records: > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5 > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2 > > > > 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 | 16 +++++++++++++--- > > 3 files changed, 20 insertions(+), 8 deletions(-) > > ... > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index d1fb821de104..3133c4175c15 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > > context->type = AUDIT_KERN_MODULE; > > } > > > > -void __audit_fanotify(u32 response) > > +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar) > > { > > - audit_log(audit_context(), GFP_KERNEL, > > - AUDIT_FANOTIFY, "resp=%u", response); > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2", > > + response, FAN_RESPONSE_INFO_NONE); > > + return; > > + } > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > + "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u", > > + response, friar->hdr.type, friar->rule_number, > > + friar->subj_trust, friar->obj_trust); > > } > > The only thing that comes to mind might be to convert the if-return > into a switch statement to make it a bit cleaner and easier to patch > in the future, but that is soooo far removed from any real concern > that I debated even mentioning it. I only bring it up in case the > "3F" discussion results in a respin, and even then I'm not going to > hold my ACK over something as silly as a if-return vs switch. > > For clarity, this is what I was thinking: > > void __audit_fanontify(...) > { > switch (type) { > case FAN_RESPONSE_INFO_NONE: > audit_log(...); > break; > default: > audit_log(...); > } > } I agree that would be cleaner, but FAN_RESPONSE_INFO_NONE and FAN_RESPONSE_INFO_AUDIT_RULE would be the two, with default being ignored since they could be other types unrelated to audit. There were a number of bits of code to future proof it in previous versions that were questioned as necessary so they were removed... > 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 Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs <rgb@redhat.com> wrote: > On 2023-01-20 13:52, Paul Moore wrote: > > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > Hello Richard, > > > > > > I built a new kernel and tested this with old and new user space. It is > > > working as advertised. The only thing I'm wondering about is why we have 3F > > > as the default value when no additional info was sent? Would it be better to > > > just make it 0? > > > > ... > > > > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote: > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > index d1fb821de104..3133c4175c15 100644 > > > > --- a/kernel/auditsc.c > > > > +++ b/kernel/auditsc.c > > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > > > > context->type = AUDIT_KERN_MODULE; > > > > } > > > > > > > > -void __audit_fanotify(u32 response) > > > > +void __audit_fanotify(u32 response, struct > > > > fanotify_response_info_audit_rule *friar) { > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 > > > obj_trust=2", > > > > + response, FAN_RESPONSE_INFO_NONE); > > > > + return; > > > > + } > > > > (I'm working under the assumption that the "fan_info=3F" in the record > > above is what Steve was referring to in his comment.) > > > > I vaguely recall Richard commenting on this in the past, although > > maybe not ... my thought is that the "3F" is simply the hex encoded > > "?" character in ASCII ('man 7 ascii' is your friend). I suppose the > > question is what to do in the FAN_RESPONSE_INFO_NONE case. > > > > Historically when we had a missing field we would follow the "field=?" > > pattern, but I don't recall doing that for a field which was > > potentially hex encoded, is there an existing case where we use "?" > > for a field that is hex encoded? If so, we can swap out the "3F" for > > a more obvious "?". > > I was presuming encoding the zero: "30" I'm sorry, but you've lost me here. > > However, another option might be to simply output the current > > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g. > > only "resp=%u". This is a little against the usual guidance of > > "fields should not disappear from a record", but considering that > > userspace will always need to support the original resp-only format > > for compatibility reasons this may be an option. > > I don't have a strong opinion. I'm not sure I care too much either. I will admit that the "3F" seems to be bordering on the "bit too clever" side of things, but it's easy to argue it is in keeping with the general idea of using "?" to denote absent/unknown fields. As Steve was the one who raised the question in this latest round, and he knows his userspace tools the best, it seems wise to get his input on this.
On Wed, Jan 25, 2023 at 5:11 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2023-01-20 13:58, Paul Moore wrote: > > On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > > > > > This patch passes the full response 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. > > > > > > Currently the only type of fanotify info that is defined is an audit > > > rule number, but convert it to hex encoding to future-proof the field. > > > Hex encoding suggested by Paul Moore <paul@paul-moore.com>. > > > > > > The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown. > > > > > > Sample records: > > > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5 > > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2 > > > > > > 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 | 16 +++++++++++++--- > > > 3 files changed, 20 insertions(+), 8 deletions(-) > > > > ... > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > index d1fb821de104..3133c4175c15 100644 > > > --- a/kernel/auditsc.c > > > +++ b/kernel/auditsc.c > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > > > context->type = AUDIT_KERN_MODULE; > > > } > > > > > > -void __audit_fanotify(u32 response) > > > +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar) > > > { > > > - audit_log(audit_context(), GFP_KERNEL, > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > > + "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2", > > > + response, FAN_RESPONSE_INFO_NONE); > > > + return; > > > + } > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > > + "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u", > > > + response, friar->hdr.type, friar->rule_number, > > > + friar->subj_trust, friar->obj_trust); > > > } > > > > The only thing that comes to mind might be to convert the if-return > > into a switch statement to make it a bit cleaner and easier to patch > > in the future, but that is soooo far removed from any real concern > > that I debated even mentioning it. I only bring it up in case the > > "3F" discussion results in a respin, and even then I'm not going to > > hold my ACK over something as silly as a if-return vs switch. > > > > For clarity, this is what I was thinking: > > > > void __audit_fanontify(...) > > { > > switch (type) { > > case FAN_RESPONSE_INFO_NONE: > > audit_log(...); > > break; > > default: > > audit_log(...); > > } > > } > > I agree that would be cleaner ... As I said, the "3F" concern of Steve is really the only thing I would bother respinning for, my other comments were just passing observations.
On Friday, January 27, 2023 3:00:37 PM EST Paul Moore wrote: > On Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > On 2023-01-20 13:52, Paul Moore wrote: > > > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <sgrubb@redhat.com> wrote: > > > > Hello Richard, > > > > > > > > I built a new kernel and tested this with old and new user space. It > > > > is > > > > working as advertised. The only thing I'm wondering about is why we > > > > have 3F as the default value when no additional info was sent? Would > > > > it be better to just make it 0? > > > > > > ... > > > > > > > On Tuesday, January 17, 2023 4:14:07 PM EST Richard Guy Briggs wrote: > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > > > > index d1fb821de104..3133c4175c15 100644 > > > > > --- a/kernel/auditsc.c > > > > > +++ b/kernel/auditsc.c > > > > > @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) > > > > > > > > > > context->type = AUDIT_KERN_MODULE; > > > > > > > > > > } > > > > > > > > > > -void __audit_fanotify(u32 response) > > > > > +void __audit_fanotify(u32 response, struct > > > > > fanotify_response_info_audit_rule *friar) { > > > > > - audit_log(audit_context(), GFP_KERNEL, > > > > > - AUDIT_FANOTIFY, "resp=%u", response); > > > > > + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ > > > > > + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { > > > > > + audit_log(audit_context(), GFP_KERNEL, > > > > > AUDIT_FANOTIFY, > > > > > + "resp=%u fan_type=%u fan_info=3F > > > > > subj_trust=2 > > > > > > > > obj_trust=2", > > > > > > > > > + response, FAN_RESPONSE_INFO_NONE); > > > > > + return; > > > > > + } > > > > > > (I'm working under the assumption that the "fan_info=3F" in the record > > > above is what Steve was referring to in his comment.) > > > > > > I vaguely recall Richard commenting on this in the past, although > > > maybe not ... my thought is that the "3F" is simply the hex encoded > > > "?" character in ASCII ('man 7 ascii' is your friend). I suppose the > > > question is what to do in the FAN_RESPONSE_INFO_NONE case. > > > > > > Historically when we had a missing field we would follow the "field=?" > > > pattern, but I don't recall doing that for a field which was > > > potentially hex encoded, is there an existing case where we use "?" > > > for a field that is hex encoded? If so, we can swap out the "3F" for > > > a more obvious "?". > > > > I was presuming encoding the zero: "30" > > I'm sorry, but you've lost me here. > > > > However, another option might be to simply output the current > > > AUDIT_FANOTIFY record format in the FAN_RESPONSE_INFO_NONE case, e.g. > > > only "resp=%u". This is a little against the usual guidance of > > > "fields should not disappear from a record", but considering that > > > userspace will always need to support the original resp-only format > > > for compatibility reasons this may be an option. > > > > I don't have a strong opinion. > > I'm not sure I care too much either. I will admit that the "3F" seems > to be bordering on the "bit too clever" side of things, but it's easy > to argue it is in keeping with the general idea of using "?" to denote > absent/unknown fields. The translation will be from %X to %u. In that case, someone might think 63 has some meaning. It would be better to leave it as 0 so there's less to explain. -Steve > As Steve was the one who raised the question in this latest round, and > he knows his userspace tools the best, it seems wise to get his input > on this.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 24ec1d66d5a8..29bdd99b29fa 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -273,7 +273,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->audit_rule); 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 d6b7d0c7ce43..31086a72e32a 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) @@ -416,7 +417,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, struct fanotify_response_info_audit_rule *friar); 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, @@ -523,10 +524,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, struct fanotify_response_info_audit_rule *friar) { if (!audit_dummy_context()) - __audit_fanotify(response); + __audit_fanotify(response, friar); } static inline void audit_tk_injoffset(struct timespec64 offset) @@ -679,7 +680,7 @@ static inline void audit_log_kern_module(char *name) { } -static inline void audit_fanotify(u32 response) +static inline void audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar) { } static inline void audit_tk_injoffset(struct timespec64 offset) diff --git a/kernel/auditsc.c b/kernel/auditsc.c index d1fb821de104..3133c4175c15 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" @@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name) context->type = AUDIT_KERN_MODULE; } -void __audit_fanotify(u32 response) +void __audit_fanotify(u32 response, struct fanotify_response_info_audit_rule *friar) { - audit_log(audit_context(), GFP_KERNEL, - AUDIT_FANOTIFY, "resp=%u", response); + /* {subj,obj}_trust values are {0,1,2}: no,yes,unknown */ + if (friar->hdr.type == FAN_RESPONSE_INFO_NONE) { + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=%u fan_info=3F subj_trust=2 obj_trust=2", + response, FAN_RESPONSE_INFO_NONE); + return; + } + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, + "resp=%u fan_type=%u fan_info=%X subj_trust=%u obj_trust=%u", + response, friar->hdr.type, friar->rule_number, + friar->subj_trust, friar->obj_trust); } void __audit_tk_injoffset(struct timespec64 offset)
This patch passes the full response 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. Currently the only type of fanotify info that is defined is an audit rule number, but convert it to hex encoding to future-proof the field. Hex encoding suggested by Paul Moore <paul@paul-moore.com>. The {subj,obj}_trust values are {0,1,2}, corresponding to no, yes, unknown. Sample records: type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F subj_trust=2 obj_trust=2 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 | 16 +++++++++++++--- 3 files changed, 20 insertions(+), 8 deletions(-)