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 <[email protected]>
Signed-off-by: Richard Guy Briggs <[email protected]>
---
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;
--
2.27.0
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 <[email protected]>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > ---
> > 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 <[email protected]>
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 <[email protected]> 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 <[email protected]>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> 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
--
paul-moore.com
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 <[email protected]>
> Signed-off-by: Richard Guy Briggs <[email protected]>
> ---
> 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 <[email protected]>
> > Signed-off-by: Richard Guy Briggs <[email protected]>
> > ---
> > 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 <[email protected]>
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