| Message ID | 2d8159cec4392029dabfc39b55ac5fbd0faa9fbd.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 |
Hell Richard, On Tuesday, August 9, 2022 1:22:55 PM EDT Richard Guy Briggs wrote: > 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. > > Sample record: > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 > fan_info=3F I compiled a new kernel and run old user space on this. The above event is exactly what I see in my audit logs. Why the fan_info=3F? I really would have expected 0. What if the actual rule number was 63? I think this will work better to leave everything 0 with old user space. -Steve > Suggested-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > kernel/auditsc.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index f000fec52360..0f747015c577 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len, > char *buf) > > if (!(len && buf)) { > audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > - "resp=%u fan_type=0 fan_info=?", response); > + "resp=%u fan_type=0 fan_info=3F", response); /* "?" */ > return; > } > while (c >= sizeof(struct fanotify_response_info_header)) { > + struct audit_context *ctx = audit_context(); > + struct audit_buffer *ab; > + > 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); > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > + if (ab) { > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > + response, friar- >hdr.type); > +#define INCOMPLETE "(incomplete)" > + audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE)); > + audit_log_end(ab); > + } > return; > } > - audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > - "resp=%u fan_type=%u fan_info=%u", > - response, friar->hdr.type, friar->audit_rule); > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > + if (ab) { > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > + response, friar->hdr.type); > + audit_log_n_hex(ab, (char *)&friar->audit_rule, > + sizeof(friar->audit_rule)); > + audit_log_end(ab); > + > + } > } > c -= friar->hdr.len; > ib += friar->hdr.len;
On 2022-08-10 15:15, Steve Grubb wrote: > Hell Richard, That's quite an introduction! ;-) > On Tuesday, August 9, 2022 1:22:55 PM EDT Richard Guy Briggs wrote: > > 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. > > > > Sample record: > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 > > fan_info=3F > > I compiled a new kernel and run old user space on this. The above event is > exactly what I see in my audit logs. Why the fan_info=3F? I really would have > expected 0. What if the actual rule number was 63? I think this will work > better to leave everything 0 with old user space. Well, if it is to be consistently hex encoded, that corresponds to "?" if it is to be interpreted as a string. Since the fan_type is 0, fan_info would be invalid, so a value of 0 would be entirely reasonable, hex encoded to fan_info=00. It could also be hex encoded to the string "(none)". If you wanted "0" for fan_type=FAN_RESPONSE_INFO_AUDIT_RULE, that would be fan_info=30 if it were interpreted as a string, or arguably 3F for an integer of rule (decimal) 63. Ultimately, fan_type should determine how fan_info's hex encoded value should be interpreted. But ultimately, the point of this patch is to hex encode the fan_info field value. > -Steve > > > Suggested-by: Paul Moore <paul@paul-moore.com> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > kernel/auditsc.c | 28 +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index f000fec52360..0f747015c577 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len, > > char *buf) > > > > if (!(len && buf)) { > > audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > - "resp=%u fan_type=0 fan_info=?", response); > > + "resp=%u fan_type=0 fan_info=3F", response); /* "?" */ > > return; > > } > > while (c >= sizeof(struct fanotify_response_info_header)) { > > + struct audit_context *ctx = audit_context(); > > + struct audit_buffer *ab; > > + > > 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); > > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > > + if (ab) { > > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > > + response, friar- > >hdr.type); > > +#define INCOMPLETE "(incomplete)" > > + audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE)); > > + audit_log_end(ab); > > + } > > return; > > } > > - audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > - "resp=%u fan_type=%u fan_info=%u", > > - response, friar->hdr.type, friar->audit_rule); > > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > > + if (ab) { > > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > > + response, friar->hdr.type); > > + audit_log_n_hex(ab, (char *)&friar->audit_rule, > > + sizeof(friar->audit_rule)); > > + audit_log_end(ab); > > + > > + } > > } > > c -= friar->hdr.len; > > ib += friar->hdr.len; > > > > - 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
Hello Richard, On Wednesday, August 10, 2022 10:23:49 PM EDT Richard Guy Briggs wrote: > > I compiled a new kernel and run old user space on this. The above event > > is > > exactly what I see in my audit logs. Why the fan_info=3F? I really would > > have expected 0. What if the actual rule number was 63? I think this > > will work better to leave everything 0 with old user space. > > Well, if it is to be consistently hex encoded, that corresponds to "?" I suppose this OK. > if it is to be interpreted as a string. Since the fan_type is 0, > fan_info would be invalid, so a value of 0 would be entirely reasonable, > hex encoded to fan_info=00. It could also be hex encoded to the string > "(none)". If you wanted "0" for fan_type=FAN_RESPONSE_INFO_AUDIT_RULE, > that would be fan_info=30 if it were interpreted as a string, or > arguably 3F for an integer of rule (decimal) 63. Ultimately, fan_type > should determine how fan_info's hex encoded value should be interpreted. > > But ultimately, the point of this patch is to hex encode the fan_info > field value. Just one last update, I have been able to test the patches with the user space application and it appears to be working from the PoV of what is sent is what's in the audit logs. I'm not sure how picky old kernels are wrt the size of what's sent. But an unpatched 5.19 kernel seems to accept the larger size response and do the right thing. -Steve
On Tue, Aug 9, 2022 at 1:23 PM Richard Guy Briggs <rgb@redhat.com> wrote: > > 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. > > Sample record: > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F > > Suggested-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > kernel/auditsc.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) This needs to be squashed with patch 3/4; it's a user visible change so we don't want someone backporting 3/4 without 4/4, especially when it is part of the same patchset. > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index f000fec52360..0f747015c577 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len, char *buf) > > if (!(len && buf)) { > audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > - "resp=%u fan_type=0 fan_info=?", response); > + "resp=%u fan_type=0 fan_info=3F", response); /* "?" */ Please drop the trailing comment, it's not necessary and it makes the code messier. > return; > } > while (c >= sizeof(struct fanotify_response_info_header)) { > + struct audit_context *ctx = audit_context(); > + struct audit_buffer *ab; > + > 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); > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > + if (ab) { > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > + response, friar->hdr.type); > +#define INCOMPLETE "(incomplete)" > + audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE)); Is the distinction between "?" and "(incomplete)" really that important? I'm not going to go digging through all of the audit_log_format() callers to check, but I believe there is precedence for using "?" not only for when a value is missing, but when it is bogus as well. If we are really going to use "(incomplete)" here, let's do a better job than defining a macro mid-function and only using it in one other place - the line immediately below the definition. This is both ugly and a little silly (especially when one considers that the macro name is almost exactly the same as the string it replaces. If we must use "(incomplete)" here, just ditch the macro; any conceptual arguments about macros vs literals is largely rendered moot since there is only one user. > + audit_log_end(ab); > + } > return; > } > - audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > - "resp=%u fan_type=%u fan_info=%u", > - response, friar->hdr.type, friar->audit_rule); > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > + if (ab) { > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > + response, friar->hdr.type); > + audit_log_n_hex(ab, (char *)&friar->audit_rule, > + sizeof(friar->audit_rule)); > + audit_log_end(ab); > + > + } > } > c -= friar->hdr.len; > ib += friar->hdr.len; > -- > 2.27.0
Hello Richard, Although I have it working, I have some comments below that might improve things. On Tuesday, August 9, 2022 1:22:55 PM EDT Richard Guy Briggs wrote: > 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. > > Sample record: > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 > fan_info=3F > > Suggested-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > kernel/auditsc.c | 28 +++++++++++++++++++++------- > 1 file changed, 21 insertions(+), 7 deletions(-) > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > index f000fec52360..0f747015c577 100644 > --- a/kernel/auditsc.c > +++ b/kernel/auditsc.c > @@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len, > char *buf) > > if (!(len && buf)) { > audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > - "resp=%u fan_type=0 fan_info=?", response); > + "resp=%u fan_type=0 fan_info=3F", response); /* "?" */ > return; > } > while (c >= sizeof(struct fanotify_response_info_header)) { > + struct audit_context *ctx = audit_context(); > + struct audit_buffer *ab; > + > 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); > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > + if (ab) { > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > + response, friar->hdr.type); > +#define INCOMPLETE "(incomplete)" > + audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE)); > + audit_log_end(ab); > + } > return; > } > - audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > - "resp=%u fan_type=%u fan_info=%u", > - response, friar->hdr.type, friar->audit_rule); > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > + if (ab) { > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > + response, friar->hdr.type); > + audit_log_n_hex(ab, (char *)&friar->audit_rule, > + sizeof(friar->audit_rule)); One thing to point out, the structure has a member audit_rule. It is probably better to call it rule_number. This is because it has nothing to do with any actual audit rule. It is a rule number meant to be recorded by the audit system. Also, that member is a __u32 type. Hex encoding that directly gives back a __u32 when decoded - which is a bit unexpected since everything else is strings. It would be better to convert the u32 to a base 10 string and then hex encode that. A buffer of 12 bytes should be sufficient. Thanks, -Steve > + audit_log_end(ab); > + > + } > } > c -= friar->hdr.len; > ib += friar->hdr.len;
On 2022-08-16 09:37, Steve Grubb wrote: > Hello Richard, > > Although I have it working, I have some comments below that might improve > things. > > On Tuesday, August 9, 2022 1:22:55 PM EDT Richard Guy Briggs wrote: > > 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. > > > > Sample record: > > type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 > > fan_info=3F > > > > Suggested-by: Paul Moore <paul@paul-moore.com> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > kernel/auditsc.c | 28 +++++++++++++++++++++------- > > 1 file changed, 21 insertions(+), 7 deletions(-) > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c > > index f000fec52360..0f747015c577 100644 > > --- a/kernel/auditsc.c > > +++ b/kernel/auditsc.c > > @@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len, > > char *buf) > > > > if (!(len && buf)) { > > audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > - "resp=%u fan_type=0 fan_info=?", response); > > + "resp=%u fan_type=0 fan_info=3F", response); /* "?" */ > > return; > > } > > while (c >= sizeof(struct fanotify_response_info_header)) { > > + struct audit_context *ctx = audit_context(); > > + struct audit_buffer *ab; > > + > > 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); > > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > > + if (ab) { > > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > > + response, friar->hdr.type); > > +#define INCOMPLETE "(incomplete)" > > + audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE)); > > + audit_log_end(ab); > > + } > > return; > > } > > - audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, > > - "resp=%u fan_type=%u fan_info=%u", > > - response, friar->hdr.type, friar->audit_rule); > > + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); > > + if (ab) { > > + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", > > + response, friar->hdr.type); > > + audit_log_n_hex(ab, (char *)&friar->audit_rule, > > + sizeof(friar->audit_rule)); > > One thing to point out, the structure has a member audit_rule. It is > probably better to call it rule_number. This is because it has nothing to > do with any actual audit rule. It is a rule number meant to be recorded by > the audit system. > > Also, that member is a __u32 type. Hex encoding that directly gives back a > __u32 when decoded - which is a bit unexpected since everything else is > strings. It would be better to convert the u32 to a base 10 string and then > hex encode that. A buffer of 12 bytes should be sufficient. Sure, these seem reasonable. > Thanks, > -Steve > > > + audit_log_end(ab); > > + > > + } > > } > > c -= friar->hdr.len; > > ib += friar->hdr.len; > > > > - 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/kernel/auditsc.c b/kernel/auditsc.c index f000fec52360..0f747015c577 100644 --- a/kernel/auditsc.c +++ b/kernel/auditsc.c @@ -2908,22 +2908,36 @@ void __audit_fanotify(u32 response, size_t len, char *buf) if (!(len && buf)) { audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, - "resp=%u fan_type=0 fan_info=?", response); + "resp=%u fan_type=0 fan_info=3F", response); /* "?" */ return; } while (c >= sizeof(struct fanotify_response_info_header)) { + struct audit_context *ctx = audit_context(); + struct audit_buffer *ab; + 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); + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); + if (ab) { + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", + response, friar->hdr.type); +#define INCOMPLETE "(incomplete)" + audit_log_n_hex(ab, INCOMPLETE, sizeof(INCOMPLETE)); + audit_log_end(ab); + } return; } - audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY, - "resp=%u fan_type=%u fan_info=%u", - response, friar->hdr.type, friar->audit_rule); + ab = audit_log_start(ctx, GFP_KERNEL, AUDIT_FANOTIFY); + if (ab) { + audit_log_format(ab, "resp=%u fan_type=%u fan_info=", + response, friar->hdr.type); + audit_log_n_hex(ab, (char *)&friar->audit_rule, + sizeof(friar->audit_rule)); + audit_log_end(ab); + + } } c -= friar->hdr.len; ib += friar->hdr.len;
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. Sample record: type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F Suggested-by: Paul Moore <paul@paul-moore.com> Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- kernel/auditsc.c | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-)