LinuxLists.cc - [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Tue, Aug 9, 2022 at 1:23 PM Richard Guy Briggs <[email protected]> wrote:
>
> This patch passes the full value 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. The following is an example of the new record
> format:
>
> type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=17
>
> 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 | 31 ++++++++++++++++++++++++++++---
> 3 files changed, 35 insertions(+), 8 deletions(-)

You've hopefully already seen the kernel test robot build warning, so
I won't bring that up again, but a few comments below ...

> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 0f36062521f4..36c3ed1af085 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -276,7 +276,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->info_len, event->info_buf);
>
> 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 3ea198a2cd59..c69efdba12ca 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)
> @@ -417,7 +418,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, size_t len, char *buf);
> 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,
> @@ -524,10 +525,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, size_t len, char *buf)
> {
> if (!audit_dummy_context())
> - __audit_fanotify(response);
> + __audit_fanotify(response, len, buf);
> }
>
> static inline void audit_tk_injoffset(struct timespec64 offset)
> @@ -684,7 +685,7 @@ static inline void audit_log_kern_module(char *name)
> {
> }
>
> -static inline void audit_fanotify(u32 response)
> +static inline void audit_fanotify(u32 response, size_t len, char *buf)
> { }
>
> static inline void audit_tk_injoffset(struct timespec64 offset)
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 433418d73584..f000fec52360 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"
>
> @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> context->type = AUDIT_KERN_MODULE;
> }
>
> -void __audit_fanotify(u32 response)
> +void __audit_fanotify(u32 response, size_t len, char *buf)
> {
> - audit_log(audit_context(), GFP_KERNEL,
> - AUDIT_FANOTIFY, "resp=%u", response);
> + struct fanotify_response_info_audit_rule *friar;
> + size_t c = len;
> + char *ib = buf;
> +
> + if (!(len && buf)) {
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=0 fan_info=?", response);
> + return;
> + }
> + while (c >= sizeof(struct fanotify_response_info_header)) {
> + friar = (struct fanotify_response_info_audit_rule *)buf;

Since the only use of this at the moment is the
fanotify_response_info_rule, why not pass the
fanotify_response_info_rule struct directly into this function? We
can always change it if we need to in the future without affecting
userspace, and it would simplify the code.

> + 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);
> + return;
> + }
> + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> + "resp=%u fan_type=%u fan_info=%u",
> + response, friar->hdr.type, friar->audit_rule);
> + }
> + c -= friar->hdr.len;
> + ib += friar->hdr.len;
> + }
> }
>
> void __audit_tk_injoffset(struct timespec64 offset)
> --
> 2.27.0

--
paul-moore.com

2022-08-31 21:39:40

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On 2022-08-15 20:22, Paul Moore wrote:
> On Tue, Aug 9, 2022 at 1:23 PM Richard Guy Briggs <[email protected]> wrote:
> >
> > This patch passes the full value 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. The following is an example of the new record
> > format:
> >
> > type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1 fan_info=17
> >
> > 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 | 31 ++++++++++++++++++++++++++++---
> > 3 files changed, 35 insertions(+), 8 deletions(-)
>
> You've hopefully already seen the kernel test robot build warning, so
> I won't bring that up again, but a few comments below ...

Yes, dealt with...

...

> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 433418d73584..f000fec52360 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"
> >
> > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > context->type = AUDIT_KERN_MODULE;
> > }
> >
> > -void __audit_fanotify(u32 response)
> > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > {
> > - audit_log(audit_context(), GFP_KERNEL,
> > - AUDIT_FANOTIFY, "resp=%u", response);
> > + struct fanotify_response_info_audit_rule *friar;
> > + size_t c = len;
> > + char *ib = buf;
> > +
> > + if (!(len && buf)) {
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=0 fan_info=?", response);
> > + return;
> > + }
> > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > + friar = (struct fanotify_response_info_audit_rule *)buf;
>
> Since the only use of this at the moment is the
> fanotify_response_info_rule, why not pass the
> fanotify_response_info_rule struct directly into this function? We
> can always change it if we need to in the future without affecting
> userspace, and it would simplify the code.

Steve, would it make any sense for there to be more than one
FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
than one rule that contributes to a notify reason? If not, would it be
reasonable to return -EINVAL if there is more than one?

> > + 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);
> > + return;
> > + }
> > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > + "resp=%u fan_type=%u fan_info=%u",
> > + response, friar->hdr.type, friar->audit_rule);
> > + }
> > + c -= friar->hdr.len;
> > + ib += friar->hdr.len;
> > + }
> > }
> >
> > void __audit_tk_injoffset(struct timespec64 offset)
>
> 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

2022-08-31 22:20:58

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > index 433418d73584..f000fec52360 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"
> > >
> > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > context->type = AUDIT_KERN_MODULE;
> > > }
> > >
> > > -void __audit_fanotify(u32 response)
> > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > {
> > > - audit_log(audit_context(), GFP_KERNEL,
> > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > + struct fanotify_response_info_audit_rule *friar;
> > > + size_t c = len;
> > > + char *ib = buf;
> > > +
> > > + if (!(len && buf)) {
> > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > + "resp=%u fan_type=0 fan_info=?", response);
> > > + return;
> > > + }
> > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > + friar = (struct fanotify_response_info_audit_rule
> > > *)buf;
> >
> > Since the only use of this at the moment is the
> > fanotify_response_info_rule, why not pass the
> > fanotify_response_info_rule struct directly into this function? We
> > can always change it if we need to in the future without affecting
> > userspace, and it would simplify the code.
>
> Steve, would it make any sense for there to be more than one
> FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> than one rule that contributes to a notify reason? If not, would it be
> reasonable to return -EINVAL if there is more than one?

I don't see a reason for sending more than one header. What is more probable
is the need to send additional data in that header. I was thinking of maybe
bit mapping it in the rule number. But I'd suggest padding the struct just in
case it needs expanding some day.

-Steev

2022-08-31 22:41:35

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On 2022-08-31 17:25, Steve Grubb wrote:
> On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > index 433418d73584..f000fec52360 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"
> > > >
> > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > context->type = AUDIT_KERN_MODULE;
> > > > }
> > > >
> > > > -void __audit_fanotify(u32 response)
> > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > {
> > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > + struct fanotify_response_info_audit_rule *friar;
> > > > + size_t c = len;
> > > > + char *ib = buf;
> > > > +
> > > > + if (!(len && buf)) {
> > > > + audit_log(audit_context(), GFP_KERNEL, AUDIT_FANOTIFY,
> > > > + "resp=%u fan_type=0 fan_info=?", response);
> > > > + return;
> > > > + }
> > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > + friar = (struct fanotify_response_info_audit_rule
> > > > *)buf;
> > >
> > > Since the only use of this at the moment is the
> > > fanotify_response_info_rule, why not pass the
> > > fanotify_response_info_rule struct directly into this function? We
> > > can always change it if we need to in the future without affecting
> > > userspace, and it would simplify the code.
> >
> > Steve, would it make any sense for there to be more than one
> > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > than one rule that contributes to a notify reason? If not, would it be
> > reasonable to return -EINVAL if there is more than one?
>
> I don't see a reason for sending more than one header. What is more probable
> is the need to send additional data in that header. I was thinking of maybe
> bit mapping it in the rule number. But I'd suggest padding the struct just in
> case it needs expanding some day.

This doesn't exactly answer my question about multiple rules
contributing to one decision.

The need for more as yet undefined information sounds like a good reason
to define a new header if that happens.

At this point, is it reasonable to throw an error if more than one RULE
header appears in a message? The way I had coded this last patchset was
to allow for more than one RULE header and each one would get its own
record in the event.

How many rules total are likely to exist?

> -Steev

- 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

2022-09-01 00:26:30

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> On 2022-08-31 17:25, Steve Grubb wrote:
> > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > index 433418d73584..f000fec52360 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"
> > > > >
> > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > context->type = AUDIT_KERN_MODULE;
> > > > > }
> > > > >
> > > > > -void __audit_fanotify(u32 response)
> > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > {
> > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > + size_t c = len;
> > > > > + char *ib = buf;
> > > > > +
> > > > > + if (!(len && buf)) {
> > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > AUDIT_FANOTIFY,
> > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > response);
> > > > > + return;
> > > > > + }
> > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > *)buf;
> > > >
> > > > Since the only use of this at the moment is the
> > > > fanotify_response_info_rule, why not pass the
> > > > fanotify_response_info_rule struct directly into this function? We
> > > > can always change it if we need to in the future without affecting
> > > > userspace, and it would simplify the code.
> > >
> > > Steve, would it make any sense for there to be more than one
> > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > than one rule that contributes to a notify reason? If not, would it be
> > > reasonable to return -EINVAL if there is more than one?
> >
> > I don't see a reason for sending more than one header. What is more
> > probable is the need to send additional data in that header. I was
> > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > padding the struct just in case it needs expanding some day.
>
> This doesn't exactly answer my question about multiple rules
> contributing to one decision.

I don't forsee that.

> The need for more as yet undefined information sounds like a good reason
> to define a new header if that happens.

It's much better to pad the struct so that the size doesn't change.

> At this point, is it reasonable to throw an error if more than one RULE
> header appears in a message?

It is a write syscall. I'd silently discard everything else and document that
in the man pages. But the fanotify maintainers should really weigh in on
this.

> The way I had coded this last patchset was to allow for more than one RULE
> header and each one would get its own record in the event.

I do not forsee a need for this.

> How many rules total are likely to exist?

Could be a thousand. But I already know some missing information we'd like to
return to user space in an audit event, so the bit mapping on the rule number
might happen. I'd suggest padding one u32 for future use.

-Steve

2022-09-01 02:09:47

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <[email protected]> wrote:
> On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > On 2022-08-31 17:25, Steve Grubb wrote:
> > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > index 433418d73584..f000fec52360 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"
> > > > > >
> > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > }
> > > > > >
> > > > > > -void __audit_fanotify(u32 response)
> > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > {
> > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > + size_t c = len;
> > > > > > + char *ib = buf;
> > > > > > +
> > > > > > + if (!(len && buf)) {
> > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > AUDIT_FANOTIFY,
> > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > response);
> > > > > > + return;
> > > > > > + }
> > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > *)buf;
> > > > >
> > > > > Since the only use of this at the moment is the
> > > > > fanotify_response_info_rule, why not pass the
> > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > can always change it if we need to in the future without affecting
> > > > > userspace, and it would simplify the code.
> > > >
> > > > Steve, would it make any sense for there to be more than one
> > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > than one rule that contributes to a notify reason? If not, would it be
> > > > reasonable to return -EINVAL if there is more than one?
> > >
> > > I don't see a reason for sending more than one header. What is more
> > > probable is the need to send additional data in that header. I was
> > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > padding the struct just in case it needs expanding some day.
> >
> > This doesn't exactly answer my question about multiple rules
> > contributing to one decision.
>
> I don't forsee that.
>
> > The need for more as yet undefined information sounds like a good reason
> > to define a new header if that happens.
>
> It's much better to pad the struct so that the size doesn't change.
>
> > At this point, is it reasonable to throw an error if more than one RULE
> > header appears in a message?
>
> It is a write syscall. I'd silently discard everything else and document that
> in the man pages. But the fanotify maintainers should really weigh in on
> this.
>
> > The way I had coded this last patchset was to allow for more than one RULE
> > header and each one would get its own record in the event.
>
> I do not forsee a need for this.
>
> > How many rules total are likely to exist?
>
> Could be a thousand. But I already know some missing information we'd like to
> return to user space in an audit event, so the bit mapping on the rule number
> might happen. I'd suggest padding one u32 for future use.

A better way to handle an expansion like that would be to have a
length/version field at the top of the struct that could be used to
determine the size and layout of the struct.

However, to be clear, my original suggestion of passing the
fanotify_response_info_rule struct internally didn't require any
additional future proofing as it is an internal implementation detail
and not something that is exposed to userspace; the function arguments
could be changed in the future and not break userspace. I'm not quite
sure how we ended up on this topic ...

--
paul-moore.com

2022-09-01 09:07:08

by Jan Kara

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wed 31-08-22 21:47:09, Paul Moore wrote:
> On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <[email protected]> wrote:
> > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > > On 2022-08-31 17:25, Steve Grubb wrote:
> > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > index 433418d73584..f000fec52360 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"
> > > > > > >
> > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > > }
> > > > > > >
> > > > > > > -void __audit_fanotify(u32 response)
> > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > > {
> > > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > > + size_t c = len;
> > > > > > > + char *ib = buf;
> > > > > > > +
> > > > > > > + if (!(len && buf)) {
> > > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > > AUDIT_FANOTIFY,
> > > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > > response);
> > > > > > > + return;
> > > > > > > + }
> > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > > *)buf;
> > > > > >
> > > > > > Since the only use of this at the moment is the
> > > > > > fanotify_response_info_rule, why not pass the
> > > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > > can always change it if we need to in the future without affecting
> > > > > > userspace, and it would simplify the code.
> > > > >
> > > > > Steve, would it make any sense for there to be more than one
> > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > > than one rule that contributes to a notify reason? If not, would it be
> > > > > reasonable to return -EINVAL if there is more than one?
> > > >
> > > > I don't see a reason for sending more than one header. What is more
> > > > probable is the need to send additional data in that header. I was
> > > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > > padding the struct just in case it needs expanding some day.
> > >
> > > This doesn't exactly answer my question about multiple rules
> > > contributing to one decision.
> >
> > I don't forsee that.
> >
> > > The need for more as yet undefined information sounds like a good reason
> > > to define a new header if that happens.
> >
> > It's much better to pad the struct so that the size doesn't change.
> >
> > > At this point, is it reasonable to throw an error if more than one RULE
> > > header appears in a message?
> >
> > It is a write syscall. I'd silently discard everything else and document that
> > in the man pages. But the fanotify maintainers should really weigh in on
> > this.
> >
> > > The way I had coded this last patchset was to allow for more than one RULE
> > > header and each one would get its own record in the event.
> >
> > I do not forsee a need for this.
> >
> > > How many rules total are likely to exist?
> >
> > Could be a thousand. But I already know some missing information we'd like to
> > return to user space in an audit event, so the bit mapping on the rule number
> > might happen. I'd suggest padding one u32 for future use.
>
> A better way to handle an expansion like that would be to have a
> length/version field at the top of the struct that could be used to
> determine the size and layout of the struct.

We already do have the 'type' and 'len' fields in
struct fanotify_response_info_header. So if audit needs to pass more
information, we can define a new 'type' and either make it replace the
current struct fanotify_response_info_audit_rule or make it expand the
information in it. At least this is how we handle similar situation when
fanotify wants to report some new bits of information to userspace.

That being said if audit wants to have u32 pad in its struct
fanotify_response_info_audit_rule for future "optional" expansion I'm not
strictly opposed to that but I don't think it is a good idea. It is tricky
to safely start using the new field. Audit subsystem can define that the
kernel currently just ignores the field. And new kernel could start using
the passed information in the additional field but that is somewhat risky
because until that moment userspace can be passing random garbage in that
unused field and thus break when running on new kernel that tries to make
sense of it. Sure you can say it is broken userspace that does not properly
initialize the padding field but history has shown us multiple times that
events like these do happen and the breakage was unpleasant enough for
users that the kernel just had to revert back to ignoring the field.

Alternatively the kernel could bail with error if the new field is non-zero
but that would block new userspace using that field from running on old
kernel. But presumably the new userspace could be handling the error and
writing another response with new field zeroed out. That would be a safe
option although not very different from defining a new response type.

Ultimately I guess I'll leave it upto audit subsystem what it wants to have
in its struct fanotify_response_info_audit_rule because for fanotify
subsystem, it is just an opaque blob it is passing.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2022-09-01 18:35:04

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Thu, Sep 1, 2022 at 3:52 AM Jan Kara <[email protected]> wrote:
> On Wed 31-08-22 21:47:09, Paul Moore wrote:
> > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <[email protected]> wrote:
> > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > > > On 2022-08-31 17:25, Steve Grubb wrote:
> > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > > index 433418d73584..f000fec52360 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"
> > > > > > > >
> > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -void __audit_fanotify(u32 response)
> > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > > > {
> > > > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > > > + size_t c = len;
> > > > > > > > + char *ib = buf;
> > > > > > > > +
> > > > > > > > + if (!(len && buf)) {
> > > > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > AUDIT_FANOTIFY,
> > > > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > > > response);
> > > > > > > > + return;
> > > > > > > > + }
> > > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > > > *)buf;
> > > > > > >
> > > > > > > Since the only use of this at the moment is the
> > > > > > > fanotify_response_info_rule, why not pass the
> > > > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > > > can always change it if we need to in the future without affecting
> > > > > > > userspace, and it would simplify the code.
> > > > > >
> > > > > > Steve, would it make any sense for there to be more than one
> > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > > > than one rule that contributes to a notify reason? If not, would it be
> > > > > > reasonable to return -EINVAL if there is more than one?
> > > > >
> > > > > I don't see a reason for sending more than one header. What is more
> > > > > probable is the need to send additional data in that header. I was
> > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > > > padding the struct just in case it needs expanding some day.
> > > >
> > > > This doesn't exactly answer my question about multiple rules
> > > > contributing to one decision.
> > >
> > > I don't forsee that.
> > >
> > > > The need for more as yet undefined information sounds like a good reason
> > > > to define a new header if that happens.
> > >
> > > It's much better to pad the struct so that the size doesn't change.
> > >
> > > > At this point, is it reasonable to throw an error if more than one RULE
> > > > header appears in a message?
> > >
> > > It is a write syscall. I'd silently discard everything else and document that
> > > in the man pages. But the fanotify maintainers should really weigh in on
> > > this.
> > >
> > > > The way I had coded this last patchset was to allow for more than one RULE
> > > > header and each one would get its own record in the event.
> > >
> > > I do not forsee a need for this.
> > >
> > > > How many rules total are likely to exist?
> > >
> > > Could be a thousand. But I already know some missing information we'd like to
> > > return to user space in an audit event, so the bit mapping on the rule number
> > > might happen. I'd suggest padding one u32 for future use.
> >
> > A better way to handle an expansion like that would be to have a
> > length/version field at the top of the struct that could be used to
> > determine the size and layout of the struct.
>
> We already do have the 'type' and 'len' fields in
> struct fanotify_response_info_header. So if audit needs to pass more
> information, we can define a new 'type' and either make it replace the
> current struct fanotify_response_info_audit_rule or make it expand the
> information in it. At least this is how we handle similar situation when
> fanotify wants to report some new bits of information to userspace.

Perfect, I didn't know that was an option from the fanotify side; I
agree that's the right approach.

> That being said if audit wants to have u32 pad in its struct
> fanotify_response_info_audit_rule for future "optional" expansion I'm not
> strictly opposed to that but I don't think it is a good idea.

Yes, I'm not a fan of padding out this way, especially when we have
better options.

> Ultimately I guess I'll leave it upto audit subsystem what it wants to have
> in its struct fanotify_response_info_audit_rule because for fanotify
> subsystem, it is just an opaque blob it is passing.

In that case, let's stick with leveraging the type/len fields in the
fanotify_response_info_header struct, that should give us all the
flexibility we need.

Richard and Steve, it sounds like Steve is already aware of additional
information that he wants to send via the
fanotify_response_info_audit_rule struct, please include that in the
next revision of this patchset. I don't want to get this merged and
then soon after have to hack in additional info.

--
paul-moore.com

2022-09-07 20:00:26

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On 2022-09-01 14:31, Paul Moore wrote:
> On Thu, Sep 1, 2022 at 3:52 AM Jan Kara <[email protected]> wrote:
> > On Wed 31-08-22 21:47:09, Paul Moore wrote:
> > > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <[email protected]> wrote:
> > > > On Wednesday, August 31, 2022 6:19:40 PM EDT Richard Guy Briggs wrote:
> > > > > On 2022-08-31 17:25, Steve Grubb wrote:
> > > > > > On Wednesday, August 31, 2022 5:07:25 PM EDT Richard Guy Briggs wrote:
> > > > > > > > > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > > > > > > > > index 433418d73584..f000fec52360 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"
> > > > > > > > >
> > > > > > > > > @@ -2899,10 +2900,34 @@ void __audit_log_kern_module(char *name)
> > > > > > > > > context->type = AUDIT_KERN_MODULE;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > -void __audit_fanotify(u32 response)
> > > > > > > > > +void __audit_fanotify(u32 response, size_t len, char *buf)
> > > > > > > > > {
> > > > > > > > > - audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > > - AUDIT_FANOTIFY, "resp=%u", response);
> > > > > > > > > + struct fanotify_response_info_audit_rule *friar;
> > > > > > > > > + size_t c = len;
> > > > > > > > > + char *ib = buf;
> > > > > > > > > +
> > > > > > > > > + if (!(len && buf)) {
> > > > > > > > > + audit_log(audit_context(), GFP_KERNEL,
> > > > > > > > > AUDIT_FANOTIFY,
> > > > > > > > > + "resp=%u fan_type=0 fan_info=?",
> > > > > > > > > response);
> > > > > > > > > + return;
> > > > > > > > > + }
> > > > > > > > > + while (c >= sizeof(struct fanotify_response_info_header)) {
> > > > > > > > > + friar = (struct fanotify_response_info_audit_rule
> > > > > > > > > *)buf;
> > > > > > > >
> > > > > > > > Since the only use of this at the moment is the
> > > > > > > > fanotify_response_info_rule, why not pass the
> > > > > > > > fanotify_response_info_rule struct directly into this function? We
> > > > > > > > can always change it if we need to in the future without affecting
> > > > > > > > userspace, and it would simplify the code.
> > > > > > >
> > > > > > > Steve, would it make any sense for there to be more than one
> > > > > > > FAN_RESPONSE_INFO_AUDIT_RULE header in a message? Could there be more
> > > > > > > than one rule that contributes to a notify reason? If not, would it be
> > > > > > > reasonable to return -EINVAL if there is more than one?
> > > > > >
> > > > > > I don't see a reason for sending more than one header. What is more
> > > > > > probable is the need to send additional data in that header. I was
> > > > > > thinking of maybe bit mapping it in the rule number. But I'd suggest
> > > > > > padding the struct just in case it needs expanding some day.
> > > > >
> > > > > This doesn't exactly answer my question about multiple rules
> > > > > contributing to one decision.
> > > >
> > > > I don't forsee that.
> > > >
> > > > > The need for more as yet undefined information sounds like a good reason
> > > > > to define a new header if that happens.
> > > >
> > > > It's much better to pad the struct so that the size doesn't change.
> > > >
> > > > > At this point, is it reasonable to throw an error if more than one RULE
> > > > > header appears in a message?
> > > >
> > > > It is a write syscall. I'd silently discard everything else and document that
> > > > in the man pages. But the fanotify maintainers should really weigh in on
> > > > this.
> > > >
> > > > > The way I had coded this last patchset was to allow for more than one RULE
> > > > > header and each one would get its own record in the event.
> > > >
> > > > I do not forsee a need for this.
> > > >
> > > > > How many rules total are likely to exist?
> > > >
> > > > Could be a thousand. But I already know some missing information we'd like to
> > > > return to user space in an audit event, so the bit mapping on the rule number
> > > > might happen. I'd suggest padding one u32 for future use.
> > >
> > > A better way to handle an expansion like that would be to have a
> > > length/version field at the top of the struct that could be used to
> > > determine the size and layout of the struct.
> >
> > We already do have the 'type' and 'len' fields in
> > struct fanotify_response_info_header. So if audit needs to pass more
> > information, we can define a new 'type' and either make it replace the
> > current struct fanotify_response_info_audit_rule or make it expand the
> > information in it. At least this is how we handle similar situation when
> > fanotify wants to report some new bits of information to userspace.
>
> Perfect, I didn't know that was an option from the fanotify side; I
> agree that's the right approach.

This is what I expected would be the way to manage changing
requirements.

> > That being said if audit wants to have u32 pad in its struct
> > fanotify_response_info_audit_rule for future "optional" expansion I'm not
> > strictly opposed to that but I don't think it is a good idea.
>
> Yes, I'm not a fan of padding out this way, especially when we have
> better options.

Agreed.

> > Ultimately I guess I'll leave it upto audit subsystem what it wants to have
> > in its struct fanotify_response_info_audit_rule because for fanotify
> > subsystem, it is just an opaque blob it is passing.
>
> In that case, let's stick with leveraging the type/len fields in the
> fanotify_response_info_header struct, that should give us all the
> flexibility we need.
>
> Richard and Steve, it sounds like Steve is already aware of additional
> information that he wants to send via the
> fanotify_response_info_audit_rule struct, please include that in the
> next revision of this patchset. I don't want to get this merged and
> then soon after have to hack in additional info.

Steve, please define the type and name of this additional field.

I'm not particularly enthusiastic of "u32 pad;"

> 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

2022-09-07 20:19:09

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote:
> > > Ultimately I guess I'll leave it upto audit subsystem what it wants to
> > > have in its struct fanotify_response_info_audit_rule because for
> > > fanotify subsystem, it is just an opaque blob it is passing.
> >
> > In that case, let's stick with leveraging the type/len fields in the
> > fanotify_response_info_header struct, that should give us all the
> > flexibility we need.
> >
> > Richard and Steve, it sounds like Steve is already aware of additional
> > information that he wants to send via the
> > fanotify_response_info_audit_rule struct, please include that in the
> > next revision of this patchset. I don't want to get this merged and
> > then soon after have to hack in additional info.
>
> Steve, please define the type and name of this additional field.

Maybe extra_data, app_data, or extra_info. Something generic that can be
reused by any application. Default to 0 if not present.

Thanks,
-Steve

2022-09-07 20:50:59

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <[email protected]> wrote:
> On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote:
> > > > Ultimately I guess I'll leave it upto audit subsystem what it wants to
> > > > have in its struct fanotify_response_info_audit_rule because for
> > > > fanotify subsystem, it is just an opaque blob it is passing.
> > >
> > > In that case, let's stick with leveraging the type/len fields in the
> > > fanotify_response_info_header struct, that should give us all the
> > > flexibility we need.
> > >
> > > Richard and Steve, it sounds like Steve is already aware of additional
> > > information that he wants to send via the
> > > fanotify_response_info_audit_rule struct, please include that in the
> > > next revision of this patchset. I don't want to get this merged and
> > > then soon after have to hack in additional info.
> >
> > Steve, please define the type and name of this additional field.
>
> Maybe extra_data, app_data, or extra_info. Something generic that can be
> reused by any application. Default to 0 if not present.

I think the point is being missed ... The idea is to not speculate on
additional fields, as discussed we have ways to handle that, the issue
was that Steve implied that he already had ideas for "things" he
wanted to add. If there are "things" that need to be added, let's do
that now, however if there is just speculation that maybe someday we
might need to add something else we can leave that until later.

--
paul-moore.com

2022-09-08 21:17:58

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <[email protected]> wrote:
> > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote:
> > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants
> > > > > to
> > > > > have in its struct fanotify_response_info_audit_rule because for
> > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > >
> > > > In that case, let's stick with leveraging the type/len fields in the
> > > > fanotify_response_info_header struct, that should give us all the
> > > > flexibility we need.
> > > >
> > > > Richard and Steve, it sounds like Steve is already aware of
> > > > additional
> > > > information that he wants to send via the
> > > > fanotify_response_info_audit_rule struct, please include that in the
> > > > next revision of this patchset. I don't want to get this merged and
> > > > then soon after have to hack in additional info.
> > >
> > > Steve, please define the type and name of this additional field.
> >
> > Maybe extra_data, app_data, or extra_info. Something generic that can be
> > reused by any application. Default to 0 if not present.
>
> I think the point is being missed ... The idea is to not speculate on
> additional fields, as discussed we have ways to handle that, the issue
> was that Steve implied that he already had ideas for "things" he
> wanted to add. If there are "things" that need to be added, let's do
> that now, however if there is just speculation that maybe someday we
> might need to add something else we can leave that until later.

This is not speculation. I know what I want to put there. I know you want to
pin it down to exactly what it is. However, when this started a couple years
back, one of the concerns was that we're building something specific to 1 user
of fanotify. And that it would be better for all future users to have a
generic facility that everyone could use if they wanted to. That's why I'm
suggesting something generic, its so this is not special purpose that doesn't
fit any other use case.

-Steve

2022-09-08 21:45:40

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <[email protected]> wrote:
> On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <[email protected]> wrote:
> > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs wrote:
> > > > > > Ultimately I guess I'll leave it upto audit subsystem what it wants
> > > > > > to
> > > > > > have in its struct fanotify_response_info_audit_rule because for
> > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > >
> > > > > In that case, let's stick with leveraging the type/len fields in the
> > > > > fanotify_response_info_header struct, that should give us all the
> > > > > flexibility we need.
> > > > >
> > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > additional
> > > > > information that he wants to send via the
> > > > > fanotify_response_info_audit_rule struct, please include that in the
> > > > > next revision of this patchset. I don't want to get this merged and
> > > > > then soon after have to hack in additional info.
> > > >
> > > > Steve, please define the type and name of this additional field.
> > >
> > > Maybe extra_data, app_data, or extra_info. Something generic that can be
> > > reused by any application. Default to 0 if not present.
> >
> > I think the point is being missed ... The idea is to not speculate on
> > additional fields, as discussed we have ways to handle that, the issue
> > was that Steve implied that he already had ideas for "things" he
> > wanted to add. If there are "things" that need to be added, let's do
> > that now, however if there is just speculation that maybe someday we
> > might need to add something else we can leave that until later.
>
> This is not speculation. I know what I want to put there. I know you want to
> pin it down to exactly what it is. However, when this started a couple years
> back, one of the concerns was that we're building something specific to 1 user
> of fanotify. And that it would be better for all future users to have a
> generic facility that everyone could use if they wanted to. That's why I'm
> suggesting something generic, its so this is not special purpose that doesn't
> fit any other use case.

Well, we are talking specifically about fanotify in this thread and
dealing with data structures that are specific to fanotify. I can
understand wanting to future proof things, but based on what we've
seen in this thread I think we are all set in this regard.

You mention that you know what you want to put in the struct, why not
share the details with all of us so we are all on the same page and
can have a proper discussion.

--
paul-moore.com

2022-09-09 02:57:12

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On 2022-09-08 22:20, Steve Grubb wrote:
> On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <[email protected]> wrote:
> > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <[email protected]> wrote:
> > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
> wrote:
> > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > > wants
> > > > > > > > to
> > > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > > for
> > > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > > >
> > > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > > the
> > > > > > > fanotify_response_info_header struct, that should give us all the
> > > > > > > flexibility we need.
> > > > > > >
> > > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > > additional
> > > > > > > information that he wants to send via the
> > > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > > the
> > > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > > and
> > > > > > > then soon after have to hack in additional info.
> > > > > >
> > > > > > Steve, please define the type and name of this additional field.
> > > > >
> > > > > Maybe extra_data, app_data, or extra_info. Something generic that can
> > > > > be
> > > > > reused by any application. Default to 0 if not present.
> > > >
> > > > I think the point is being missed ... The idea is to not speculate on
> > > > additional fields, as discussed we have ways to handle that, the issue
> > > > was that Steve implied that he already had ideas for "things" he
> > > > wanted to add. If there are "things" that need to be added, let's do
> > > > that now, however if there is just speculation that maybe someday we
> > > > might need to add something else we can leave that until later.
> > >
> > > This is not speculation. I know what I want to put there. I know you want
> > > to pin it down to exactly what it is. However, when this started a
> > > couple years back, one of the concerns was that we're building something
> > > specific to 1 user of fanotify. And that it would be better for all
> > > future users to have a generic facility that everyone could use if they
> > > wanted to. That's why I'm suggesting something generic, its so this is
> > > not special purpose that doesn't fit any other use case.
> >
> > Well, we are talking specifically about fanotify in this thread and
> > dealing with data structures that are specific to fanotify. I can
> > understand wanting to future proof things, but based on what we've
> > seen in this thread I think we are all set in this regard.
>
> I'm trying to abide by what was suggested by the fs-devel folks. I can live
> with it. But if you want to make something non-generic for all users of
> fanotify, call the new field "trusted". This would decern when a decision was
> made because the file was untrusted or access denied for another reason.

So, "u32 trusted;" ? How would you like that formatted?
"fan_trust={0|1}"

> > You mention that you know what you want to put in the struct, why not
> > share the details with all of us so we are all on the same page and
> > can have a proper discussion.
>
> Because I want to abide by the original agreement and not impose opinionated
> requirements that serve no one else. I'd rather have something anyone can
> use. I want to play nice.

If someone else wants to use something, why not give them a type of
their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
however they like?

> -Steve

- 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

2022-09-09 02:58:55

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <[email protected]> wrote:
> > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <[email protected]> wrote:
> > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
wrote:
> > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > wants
> > > > > > > to
> > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > for
> > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > >
> > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > the
> > > > > > fanotify_response_info_header struct, that should give us all the
> > > > > > flexibility we need.
> > > > > >
> > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > additional
> > > > > > information that he wants to send via the
> > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > the
> > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > and
> > > > > > then soon after have to hack in additional info.
> > > > >
> > > > > Steve, please define the type and name of this additional field.
> > > >
> > > > Maybe extra_data, app_data, or extra_info. Something generic that can
> > > > be
> > > > reused by any application. Default to 0 if not present.
> > >
> > > I think the point is being missed ... The idea is to not speculate on
> > > additional fields, as discussed we have ways to handle that, the issue
> > > was that Steve implied that he already had ideas for "things" he
> > > wanted to add. If there are "things" that need to be added, let's do
> > > that now, however if there is just speculation that maybe someday we
> > > might need to add something else we can leave that until later.
> >
> > This is not speculation. I know what I want to put there. I know you want
> > to pin it down to exactly what it is. However, when this started a
> > couple years back, one of the concerns was that we're building something
> > specific to 1 user of fanotify. And that it would be better for all
> > future users to have a generic facility that everyone could use if they
> > wanted to. That's why I'm suggesting something generic, its so this is
> > not special purpose that doesn't fit any other use case.
>
> Well, we are talking specifically about fanotify in this thread and
> dealing with data structures that are specific to fanotify. I can
> understand wanting to future proof things, but based on what we've
> seen in this thread I think we are all set in this regard.

I'm trying to abide by what was suggested by the fs-devel folks. I can live
with it. But if you want to make something non-generic for all users of
fanotify, call the new field "trusted". This would decern when a decision was
made because the file was untrusted or access denied for another reason.

> You mention that you know what you want to put in the struct, why not
> share the details with all of us so we are all on the same page and
> can have a proper discussion.

Because I want to abide by the original agreement and not impose opinionated
requirements that serve no one else. I'd rather have something anyone can
use. I want to play nice.

-Steve

2022-09-09 04:17:25

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Thu, Sep 8, 2022 at 10:41 PM Richard Guy Briggs <[email protected]> wrote:
> On 2022-09-08 22:20, Steve Grubb wrote:
> > On Thursday, September 8, 2022 5:22:15 PM EDT Paul Moore wrote:
> > > On Thu, Sep 8, 2022 at 5:14 PM Steve Grubb <[email protected]> wrote:
> > > > On Wednesday, September 7, 2022 4:23:49 PM EDT Paul Moore wrote:
> > > > > On Wed, Sep 7, 2022 at 4:11 PM Steve Grubb <[email protected]> wrote:
> > > > > > On Wednesday, September 7, 2022 2:43:54 PM EDT Richard Guy Briggs
> > wrote:
> > > > > > > > > Ultimately I guess I'll leave it upto audit subsystem what it
> > > > > > > > > wants
> > > > > > > > > to
> > > > > > > > > have in its struct fanotify_response_info_audit_rule because
> > > > > > > > > for
> > > > > > > > > fanotify subsystem, it is just an opaque blob it is passing.
> > > > > > > >
> > > > > > > > In that case, let's stick with leveraging the type/len fields in
> > > > > > > > the
> > > > > > > > fanotify_response_info_header struct, that should give us all the
> > > > > > > > flexibility we need.
> > > > > > > >
> > > > > > > > Richard and Steve, it sounds like Steve is already aware of
> > > > > > > > additional
> > > > > > > > information that he wants to send via the
> > > > > > > > fanotify_response_info_audit_rule struct, please include that in
> > > > > > > > the
> > > > > > > > next revision of this patchset. I don't want to get this merged
> > > > > > > > and
> > > > > > > > then soon after have to hack in additional info.
> > > > > > >
> > > > > > > Steve, please define the type and name of this additional field.
> > > > > >
> > > > > > Maybe extra_data, app_data, or extra_info. Something generic that can
> > > > > > be
> > > > > > reused by any application. Default to 0 if not present.
> > > > >
> > > > > I think the point is being missed ... The idea is to not speculate on
> > > > > additional fields, as discussed we have ways to handle that, the issue
> > > > > was that Steve implied that he already had ideas for "things" he
> > > > > wanted to add. If there are "things" that need to be added, let's do
> > > > > that now, however if there is just speculation that maybe someday we
> > > > > might need to add something else we can leave that until later.
> > > >
> > > > This is not speculation. I know what I want to put there. I know you want
> > > > to pin it down to exactly what it is. However, when this started a
> > > > couple years back, one of the concerns was that we're building something
> > > > specific to 1 user of fanotify. And that it would be better for all
> > > > future users to have a generic facility that everyone could use if they
> > > > wanted to. That's why I'm suggesting something generic, its so this is
> > > > not special purpose that doesn't fit any other use case.
> > >
> > > Well, we are talking specifically about fanotify in this thread and
> > > dealing with data structures that are specific to fanotify. I can
> > > understand wanting to future proof things, but based on what we've
> > > seen in this thread I think we are all set in this regard.
> >
> > I'm trying to abide by what was suggested by the fs-devel folks. I can live
> > with it. But if you want to make something non-generic for all users of
> > fanotify, call the new field "trusted". This would decern when a decision was
> > made because the file was untrusted or access denied for another reason.
>
> So, "u32 trusted;" ? How would you like that formatted?
> "fan_trust={0|1}"
>
> > > You mention that you know what you want to put in the struct, why not
> > > share the details with all of us so we are all on the same page and
> > > can have a proper discussion.
> >
> > Because I want to abide by the original agreement and not impose opinionated
> > requirements that serve no one else. I'd rather have something anyone can
> > use. I want to play nice.
>
> If someone else wants to use something, why not give them a type of
> their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
> however they like?

Yes, exactly. The struct is very clearly specific to both fanotify
and audit, I see no reason why it needs to be made generic for use by
other subsystems when other mechanisms exist to support them.

--
paul-moore.com

2022-09-09 04:19:22

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > I'm trying to abide by what was suggested by the fs-devel folks. I can
> > live with it. But if you want to make something non-generic for all
> > users of fanotify, call the new field "trusted". This would decern when
> > a decision was made because the file was untrusted or access denied for
> > another reason.
>
> So, "u32 trusted;" ? How would you like that formatted?
> "fan_trust={0|1}"

So how does this play out if there is another user? Do they want a num= and
trust= if not, then the AUDIT_FANOTIFY record will have multiple formats
which is not good. I'd rather suggest something generic that can be
interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and
then followed by info0= info1= the interpretation of those solely depend on
fan_type. If the fan_type does not need both, then any interpretation skips
what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is
for that format. But make this pivot on fan_type and not actual names.

> > > You mention that you know what you want to put in the struct, why not
> > > share the details with all of us so we are all on the same page and
> > > can have a proper discussion.
> >
> > Because I want to abide by the original agreement and not impose
> > opinionated requirements that serve no one else. I'd rather have
> > something anyone can use. I want to play nice.
>
> If someone else wants to use something, why not give them a type of
> their own other than FAN_RESPONSE_INFO_AUDIT_RULE that they can shape
> however they like?

Please, let's keep AUDIT_FANOTIFY normalized but pivot on fan_type.

-Steve

2022-09-09 11:18:06

by Jan Kara

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

Hello Steve!

On Fri 09-09-22 00:03:53, Steve Grubb wrote:
> On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > > I'm trying to abide by what was suggested by the fs-devel folks. I can
> > > live with it. But if you want to make something non-generic for all
> > > users of fanotify, call the new field "trusted". This would decern when
> > > a decision was made because the file was untrusted or access denied for
> > > another reason.
> >
> > So, "u32 trusted;" ? How would you like that formatted?
> > "fan_trust={0|1}"
>
> So how does this play out if there is another user? Do they want a num= and
> trust= if not, then the AUDIT_FANOTIFY record will have multiple formats
> which is not good. I'd rather suggest something generic that can be
> interpreted based on who's attached to fanotify. IOW we have a fan_type=0 and
> then followed by info0= info1= the interpretation of those solely depend on
> fan_type. If the fan_type does not need both, then any interpretation skips
> what it doesn't need. If fan_type=1, then it follows what arg0= and arg1= is
> for that format. But make this pivot on fan_type and not actual names.

So I think there is some misunderstanding so let me maybe spell out in
detail how I see things so that we can get on the same page:

It was a requirement from me (and probably Amir) that there is a generic
way to attach additional info to a response to fanotify permission event.
This is achieved by defining:

struct fanotify_response_info_header {
__u8 type;
__u8 pad;
__u16 len;
};

which is a generic header and kernel can based on 'len' field decide how
large the response structure is (to safely copy it from userspace) and
based on 'type' field it can decide who should be the recipient of this
extra information (or generally what to do with it). So any additional
info needs to start with this header.

Then there is:

struct fanotify_response_info_audit_rule {
struct fanotify_response_info_header hdr;
__u32 audit_rule;
};

which properly starts with the header and hdr.type is expected to be
FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type
FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit*
subsystem's responsibility. Fanotify code will just pass this as an opaque
blob to the audit subsystem.

So if you know audit subsystem will also need some other field together
with 'audit_rule' now is a good time to add it and it doesn't have to be
useful for anybody else besides audit. If someone else will need other
information passed along with the response, he will append structure with
another header with different 'type' field. In principle, there can be
multiple structures appended to fanotify response like

<hdr> <data> <hdr> <data> ...

and fanotify subsystem will just pass them to different receivers based
on the type in 'hdr' field.

Also if audit needs to pass even more information along with the respose,
we can define a new 'type' for it. But the 'type' space is not infinite so
I'd prefer this does not happen too often...

I hope this clears out things a bit.

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR

2022-09-09 14:29:16

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Friday, September 9, 2022 7:09:44 AM EDT Jan Kara wrote:
> Hello Steve!
>
> On Fri 09-09-22 00:03:53, Steve Grubb wrote:
> > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > > > I'm trying to abide by what was suggested by the fs-devel folks. I
> > > > can
> > > > live with it. But if you want to make something non-generic for all
> > > > users of fanotify, call the new field "trusted". This would decern
> > > > when
> > > > a decision was made because the file was untrusted or access denied
> > > > for
> > > > another reason.
> > >
> > > So, "u32 trusted;" ? How would you like that formatted?
> > > "fan_trust={0|1}"
> >
> > So how does this play out if there is another user? Do they want a num=
> > and trust= if not, then the AUDIT_FANOTIFY record will have multiple
> > formats which is not good. I'd rather suggest something generic that can
> > be interpreted based on who's attached to fanotify. IOW we have a
> > fan_type=0 and then followed by info0= info1= the interpretation of
> > those solely depend on fan_type. If the fan_type does not need both,
> > then any interpretation skips what it doesn't need. If fan_type=1, then
> > it follows what arg0= and arg1= is for that format. But make this pivot
> > on fan_type and not actual names.
> So I think there is some misunderstanding so let me maybe spell out in
> detail how I see things so that we can get on the same page:
>
> It was a requirement from me (and probably Amir) that there is a generic
> way to attach additional info to a response to fanotify permission event.
> This is achieved by defining:
>
> struct fanotify_response_info_header {
> __u8 type;
> __u8 pad;
> __u16 len;
> };
>
> which is a generic header and kernel can based on 'len' field decide how
> large the response structure is (to safely copy it from userspace) and
> based on 'type' field it can decide who should be the recipient of this
> extra information (or generally what to do with it). So any additional
> info needs to start with this header.
>
> Then there is:
>
> struct fanotify_response_info_audit_rule {
> struct fanotify_response_info_header hdr;
> __u32 audit_rule;
> };
>
> which properly starts with the header and hdr.type is expected to be
> FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type
> FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit*
> subsystem's responsibility. Fanotify code will just pass this as an opaque
> blob to the audit subsystem.
>
> So if you know audit subsystem will also need some other field together
> with 'audit_rule' now is a good time to add it and it doesn't have to be
> useful for anybody else besides audit. If someone else will need other
> information passed along with the response, he will append structure with
> another header with different 'type' field. In principle, there can be
> multiple structures appended to fanotify response like
>
> <hdr> <data> <hdr> <data> ...
>
> and fanotify subsystem will just pass them to different receivers based
> on the type in 'hdr' field.
>
> Also if audit needs to pass even more information along with the respose,
> we can define a new 'type' for it. But the 'type' space is not infinite so
> I'd prefer this does not happen too often...
>
> I hope this clears out things a bit.

Yes. Thank you.

Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes,
unknown.

-Steve

2022-09-09 15:00:52

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On 2022-09-09 10:22, Steve Grubb wrote:
> On Friday, September 9, 2022 7:09:44 AM EDT Jan Kara wrote:
> > Hello Steve!
> >
> > On Fri 09-09-22 00:03:53, Steve Grubb wrote:
> > > On Thursday, September 8, 2022 10:41:44 PM EDT Richard Guy Briggs wrote:
> > > > > I'm trying to abide by what was suggested by the fs-devel folks. I
> > > > > can
> > > > > live with it. But if you want to make something non-generic for all
> > > > > users of fanotify, call the new field "trusted". This would decern
> > > > > when
> > > > > a decision was made because the file was untrusted or access denied
> > > > > for
> > > > > another reason.
> > > >
> > > > So, "u32 trusted;" ? How would you like that formatted?
> > > > "fan_trust={0|1}"
> > >
> > > So how does this play out if there is another user? Do they want a num=
> > > and trust= if not, then the AUDIT_FANOTIFY record will have multiple
> > > formats which is not good. I'd rather suggest something generic that can
> > > be interpreted based on who's attached to fanotify. IOW we have a
> > > fan_type=0 and then followed by info0= info1= the interpretation of
> > > those solely depend on fan_type. If the fan_type does not need both,
> > > then any interpretation skips what it doesn't need. If fan_type=1, then
> > > it follows what arg0= and arg1= is for that format. But make this pivot
> > > on fan_type and not actual names.
> > So I think there is some misunderstanding so let me maybe spell out in
> > detail how I see things so that we can get on the same page:
> >
> > It was a requirement from me (and probably Amir) that there is a generic
> > way to attach additional info to a response to fanotify permission event.
> > This is achieved by defining:
> >
> > struct fanotify_response_info_header {
> > __u8 type;
> > __u8 pad;
> > __u16 len;
> > };
> >
> > which is a generic header and kernel can based on 'len' field decide how
> > large the response structure is (to safely copy it from userspace) and
> > based on 'type' field it can decide who should be the recipient of this
> > extra information (or generally what to do with it). So any additional
> > info needs to start with this header.
> >
> > Then there is:
> >
> > struct fanotify_response_info_audit_rule {
> > struct fanotify_response_info_header hdr;
> > __u32 audit_rule;
> > };
> >
> > which properly starts with the header and hdr.type is expected to be
> > FAN_RESPONSE_INFO_AUDIT_RULE. What happens after the header with type
> > FAN_RESPONSE_INFO_AUDIT_RULE until length hdr.len is fully within *audit*
> > subsystem's responsibility. Fanotify code will just pass this as an opaque
> > blob to the audit subsystem.
> >
> > So if you know audit subsystem will also need some other field together
> > with 'audit_rule' now is a good time to add it and it doesn't have to be
> > useful for anybody else besides audit. If someone else will need other
> > information passed along with the response, he will append structure with
> > another header with different 'type' field. In principle, there can be
> > multiple structures appended to fanotify response like
> >
> > <hdr> <data> <hdr> <data> ...
> >
> > and fanotify subsystem will just pass them to different receivers based
> > on the type in 'hdr' field.
> >
> > Also if audit needs to pass even more information along with the respose,
> > we can define a new 'type' for it. But the 'type' space is not infinite so
> > I'd prefer this does not happen too often...
> >
> > I hope this clears out things a bit.
>
> Yes. Thank you.
>
> Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes,
> unknown.

type? bitfield? My gut would say that "0" should be "unset"/"unknown",
but that is counterintuitive to the values represented.

Or "trust" with sub-fields "subj" and "obj"?

> -Steve

- 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

2022-09-09 15:30:44

[permalink] [raw]

Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full permission event response

On Friday, September 9, 2022 10:38:46 AM EDT Richard Guy Briggs wrote:
> > Richard, add subj_trust and obj_trust. These can be 0|1|2 for no, yes,
> > unknown.
>
> type? bitfield? My gut would say that "0" should be "unset"/"unknown",
> but that is counterintuitive to the values represented.
>
> Or "trust" with sub-fields "subj" and "obj"?

No. just make them separate and u32. subj_trust and obj_trust - no sub fields.
If we have sub-fields, that probably means bit mapping and that wasn't wanted.

-Steve

2022-09-09 19:13:55