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=?
changelog:
v1:
- first version by Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/2042449.irdbgypaU6@x2
v2:
- enhancements suggested by Jan Kara <[email protected]>
- 1/3 change %d to %u in pr_debug
- 2/3 change response from __u32 to __u16
- mod struct fanotify_response and fanotify_perm_event add extra_info_type, extra_info_buf
- extra_info_buf size max FANOTIFY_MAX_RESPONSE_EXTRA_LEN, add struct fanotify_response_audit_rule
- extend debug statements
- remove unneeded macros
- [internal] change interface to finish_permission_event() and process_access_response()
- 3/3 update format of extra information
- [internal] change interface to audit_fanotify()
- change ctx_type= to fan_type=
Link: https://lore.kernel.org/r/[email protected]
v3:
- 1/3 switch {,__}audit_fanotify() from uint to u32
- 2/3 re-add fanotify_get_response switch case FAN_DENY: to avoid unnecessary churn
- add FAN_EXTRA flag to indicate more info and break with old kernel
- change response from u16 to u32 to avoid endian issues
- change extra_info_buf to union
- move low-cost fd check earlier
- change FAN_RESPONSE_INFO_AUDIT_NONE to FAN_RESPONSE_INFO_NONE
- switch to u32 for internal and __u32 for uapi
Link: https://lore.kernel.org/all/[email protected]
v4:
- scrap FAN_INVALID_RESPONSE_MASK in favour of original to catch invalid response == 0
- introduce FANOTIFY_RESPONSE_* macros
- uapi: remove union
- keep original struct fanotify_response, add fan_info infra starting with audit reason
- uapi add struct fanotify_response_info_header{type/pad/len} and struct fanotify_response_info_audit_rule{hdr/rule}
- rename fan_ctx= to fan_info=, FAN_EXTRA to FAN_INFO
- change event struct from type/buf to len/buf
- enable multiple info extensions in one message
- hex encode fan_info in __audit_fanotify()
- record type FANOTIFY extended to "type=FANOTIFY msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=3F"
Link: https://lore.kernel.org/all/[email protected]
v5:
- fixed warnings in p2/4 and p3/4 found by <[email protected]>
- restore original behaviour for !FAN_INFO case and fanotify_get_response()
- rename member audit_rule to rule_number
- eliminate memory leak of info_buf on failure (no longer dynamic)
- rename buf:info, count:info_len, c:remain, ib:infop
- fix pr_debug
- return -ENOENT on FAN_INFO and fd==FAN_NOFD to signal new kernel
- fanotify_write() remove redundant size check
- add u32 subj_trust obj_trust fields with unknown value "2"
- split out to helper process_access_response_info()
- restore finish_permission_event() response_struct to u32
- assume and enforce one rule to audit, pass struct directly to __audit_fanotify()
- change fanotify_perm_event struct to union hdr/audir_rule
- add vspace to fanotify_write() and process_access_response_info()
- squash 3/4 with 4/4
- fix v3 and v4 links
Link: https://lore.kernel.org/all/[email protected]
v6:
- simplify __audit_fanotify() from audit_log_format/audit_log_n_hex to audit_log/%X
- add comment to clarify {subj,obj}_trust values
- remove fd processing from process_access_response_info()
- return info_len immediately from process_access_response() on FAN_NOFD after process_access_response_info()
Link: https://lore.kernel.org/all/[email protected]
Richard Guy Briggs (3):
fanotify: Ensure consistent variable type for response
fanotify: define struct members to hold response decision context
fanotify,audit: Allow audit to use the full permission event response
fs/notify/fanotify/fanotify.c | 8 ++-
fs/notify/fanotify/fanotify.h | 6 +-
fs/notify/fanotify/fanotify_user.c | 88 ++++++++++++++++++++++--------
include/linux/audit.h | 9 +--
include/linux/fanotify.h | 5 ++
include/uapi/linux/fanotify.h | 30 +++++++++-
kernel/auditsc.c | 16 +++++-
7 files changed, 129 insertions(+), 33 deletions(-)
--
2.27.0
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
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(-)
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)
--
2.27.0
The user space API for the response variable is __u32. This patch makes
sure that the whole path through the kernel uses u32 so that there is
no sign extension or truncation of the user space response.
Suggested-by: Steve Grubb <[email protected]>
Link: https://lore.kernel.org/r/12617626.uLZWGnKmhe@x2
Signed-off-by: Richard Guy Briggs <[email protected]>
Acked-by: Paul Moore <[email protected]>
---
fs/notify/fanotify/fanotify.h | 2 +-
fs/notify/fanotify/fanotify_user.c | 6 +++---
include/linux/audit.h | 6 +++---
kernel/auditsc.c | 2 +-
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
index 57f51a9a3015..f899d610bc08 100644
--- a/fs/notify/fanotify/fanotify.h
+++ b/fs/notify/fanotify/fanotify.h
@@ -425,7 +425,7 @@ FANOTIFY_PE(struct fanotify_event *event)
struct fanotify_perm_event {
struct fanotify_event fae;
struct path path;
- unsigned short response; /* userspace answer to the event */
+ u32 response; /* userspace answer to the event */
unsigned short state; /* state of the event */
int fd; /* fd we passed to userspace for this event */
};
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 4546da4a54f9..caa1211bac8c 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -289,7 +289,7 @@ static int create_fd(struct fsnotify_group *group, const struct path *path,
*/
static void finish_permission_event(struct fsnotify_group *group,
struct fanotify_perm_event *event,
- unsigned int response)
+ u32 response)
__releases(&group->notification_lock)
{
bool destroy = false;
@@ -310,9 +310,9 @@ static int process_access_response(struct fsnotify_group *group,
{
struct fanotify_perm_event *event;
int fd = response_struct->fd;
- int response = response_struct->response;
+ u32 response = response_struct->response;
- pr_debug("%s: group=%p fd=%d response=%d\n", __func__, group,
+ pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
fd, response);
/*
* make sure the response is valid, if invalid we do nothing and either
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 3608992848d3..d6b7d0c7ce43 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -416,7 +416,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(unsigned int response);
+extern void __audit_fanotify(u32 response);
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,7 +523,7 @@ static inline void audit_log_kern_module(char *name)
__audit_log_kern_module(name);
}
-static inline void audit_fanotify(unsigned int response)
+static inline void audit_fanotify(u32 response)
{
if (!audit_dummy_context())
__audit_fanotify(response);
@@ -679,7 +679,7 @@ static inline void audit_log_kern_module(char *name)
{
}
-static inline void audit_fanotify(unsigned int response)
+static inline void audit_fanotify(u32 response)
{ }
static inline void audit_tk_injoffset(struct timespec64 offset)
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 547c88be8a28..d1fb821de104 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2877,7 +2877,7 @@ void __audit_log_kern_module(char *name)
context->type = AUDIT_KERN_MODULE;
}
-void __audit_fanotify(unsigned int response)
+void __audit_fanotify(u32 response)
{
audit_log(audit_context(), GFP_KERNEL,
AUDIT_FANOTIFY, "resp=%u", response);
--
2.27.0
On Tue 17-01-23 16:14:04, Richard Guy Briggs wrote:
> 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=?
Everything looks fine to me in this patchset so once patch 3/3 gets ack
from audit guys, I'll merge the series to my tree. Thanks for your work!
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
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 <[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
> 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(-)
>
> 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 <[email protected]> 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 <[email protected]> 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 <[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
> 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(-)
...
> 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(...);
}
}
--
paul-moore.com
On 2023-01-20 13:52, Paul Moore wrote:
> On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <[email protected]> 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 <[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
On 2023-01-20 13:58, Paul Moore wrote:
> On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs <[email protected]> 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 <[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
> > 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(-)
>
> ...
>
> > 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 <[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
On Wed, Jan 25, 2023 at 5:06 PM Richard Guy Briggs <[email protected]> wrote:
> On 2023-01-20 13:52, Paul Moore wrote:
> > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <[email protected]> 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.
--
paul-moore.com
On Wed, Jan 25, 2023 at 5:11 PM Richard Guy Briggs <[email protected]> wrote:
>
> On 2023-01-20 13:58, Paul Moore wrote:
> > On Tue, Jan 17, 2023 at 4:14 PM Richard Guy Briggs <[email protected]> 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 <[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
> > > 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(-)
> >
> > ...
> >
> > > 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.
--
paul-moore.com
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 <[email protected]> wrote:
> > On 2023-01-20 13:52, Paul Moore wrote:
> > > On Wed, Jan 18, 2023 at 1:34 PM Steve Grubb <[email protected]> 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.