fanotify: Allow user space to pass back additional audit info

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 <[email protected]>.

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 <[email protected]>

Link https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/notify/fanotify/fanotify.c |  3 ++-
 include/linux/audit.h         |  9 +++++----
 kernel/auditsc.c              | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 8 deletions(-)

fs/notify/fanotify/fanotify.c +2 -1

index 24ec1d66d5a8..29bdd99b29fa 100644
@@ -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__,

fs/notify/fanotify/fanotify.c +0 -0

 		 group, event, ret);

include/linux/audit.h +2 -1

index d6b7d0c7ce43..31086a72e32a 100644
@@ -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);
}

include/linux/audit.h +1 -1

-static inline void audit_fanotify(u32 response)
+static inline void audit_fanotify(u32 response, struct

fanotify_response_info_audit_rule *friar) {

include/linux/audit.h +2 -2

 	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)

kernel/auditsc.c +1 -0

index d1fb821de104..3133c4175c15 100644
@@ -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;
 }

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?

Quoted 8 lines

kernel/auditsc.c +0 -0

index d1fb821de104..3133c4175c15 100644
@@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
      context->type = AUDIT_KERN_MODULE;
 }
 

kernel/auditsc.c void __audit_log_kern_module(char *name)

Quoted 9 lines

kernel/auditsc.c +5 -3

-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",

Quoted 3 lines

kernel/auditsc.c +3 -0

+                       response, FAN_RESPONSE_INFO_NONE);
+             return;
+     }

Quoted 6 lines · Steve Grubb

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?

...

Quoted 9 lines · EST Richard Guy Briggs

Quoted 8 lines

kernel/auditsc.c +0 -0

index d1fb821de104..3133c4175c15 100644
@@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
      context->type = AUDIT_KERN_MODULE;
 }
 

Quoted 13 lines

kernel/auditsc.c void __audit_log_kern_module(char *name)

Quoted 9 lines

kernel/auditsc.c +5 -3

-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",

Quoted 3 lines

kernel/auditsc.c +3 -0

+                       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

Quoted 41 lines · Paul Moore

Quoted 6 lines · Steve Grubb

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?

...

Quoted 9 lines · EST Richard Guy Briggs

Quoted 8 lines

kernel/auditsc.c +0 -0

index d1fb821de104..3133c4175c15 100644
@@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
      context->type = AUDIT_KERN_MODULE;
 }
 

Quoted 13 lines

kernel/auditsc.c void __audit_log_kern_module(char *name)

Quoted 9 lines

kernel/auditsc.c +5 -3

-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",

Quoted 3 lines

kernel/auditsc.c +3 -0

+                       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.

Quoted 5 lines

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"

Quoted 6 lines

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.

Quoted 49 lines · Richard Guy Briggs

Quoted 48 lines · Paul Moore

Quoted 7 lines · Steve Grubb

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?

...

Quoted 11 lines · EST Richard Guy Briggs

Quoted 10 lines

kernel/auditsc.c +0 -0

index d1fb821de104..3133c4175c15 100644
@@ -2877,10 +2878,19 @@ void __audit_log_kern_module(char *name)
 
      context->type = AUDIT_KERN_MODULE;
 
 }
 

Quoted 17 lines

Quoted 11 lines

kernel/auditsc.c +5 -3

-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",

Quoted 3 lines

kernel/auditsc.c +3 -0

+                       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.

Quoted 7 lines

Quoted 5 lines

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.

Quoted 7 lines

Quoted 6 lines

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.

Quoted 1 line

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.

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 <[email protected]>.

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 <[email protected]>

Link https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/notify/fanotify/fanotify.c |  3 ++-
 include/linux/audit.h         |  9 +++++----
 kernel/auditsc.c              | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 8 deletions(-)

kernel/auditsc.c +3 -3

index d1fb821de104..3133c4175c15 100644
@@ -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) {

Quoted 23 lines · Richard Guy Briggs

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 <[email protected]>.

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 <[email protected]>

Link https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/notify/fanotify/fanotify.c |  3 ++-
 include/linux/audit.h         |  9 +++++----
 kernel/auditsc.c              | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 8 deletions(-)

...

Quoted 15 lines

kernel/auditsc.c +3 -3

index d1fb821de104..3133c4175c15 100644
@@ -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) {

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2894

Quoted 10 lines

kernel/auditsc.c +9 -0

+               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(...);

}
}

paul-moore.com

Quoted 66 lines · Paul Moore

Quoted 23 lines · Richard Guy Briggs

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 <[email protected]>.

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 <[email protected]>

Link https://lore.kernel.org/r/3075502.aeNJFYEL58@x2 open here

Signed-off-by Richard Guy Briggs <[email protected]>

---
 fs/notify/fanotify/fanotify.c |  3 ++-
 include/linux/audit.h         |  9 +++++----
 kernel/auditsc.c              | 16 +++++++++++++---
 3 files changed, 20 insertions(+), 8 deletions(-)

...

Quoted 15 lines

kernel/auditsc.c +3 -3

index d1fb821de104..3133c4175c15 100644
@@ -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) {

kernel/auditsc.c void __audit_log_kern_module(char *name) · line 2894

Quoted 10 lines

kernel/auditsc.c +9 -0

+               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:

Quoted 6 lines

    audit_log(...);
    break;
  default:
    audit_log(...);

}
}

I agree that would be cleaner ...

The Fanotify API can be used for access control by requesting permission event notification. The user space tooling that uses it may have a complicated policy that inherently contains additional context for the decision. If this information were available in the audit trail, policy writers can close the loop on debugging policy. Also, if this additional information were available, it would enable the creation of tools that can suggest changes to the policy similar to how audit2allow can help refine labeled security.

This patchset defines a new flag (FAN_INFO) and new extensions that define additional information which are appended after the response structure returned from user space on a permission event. The appended information is organized with headers containing a type and size that can be delegated to interested subsystems. One new information type is defined to audit the triggering rule number.

A newer kernel will work with an older userspace and an older kernel will behave as expected and reject a newer userspace, leaving it up to the newer userspace to test appropriately and adapt as necessary. This is done by providing a a fully-formed FAN_INFO extension but setting the fd to FAN_NOFD. On a capable kernel, it will succeed but issue no audit record, whereas on an older kernel it will fail.

The audit function was updated to log the additional information in the
AUDIT_FANOTIFY record. The following are examples of the new record
format:
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=? obj_trust=?