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

[Richard Guy Briggs]

On 2022-09-01 14:31, Paul Moore wrote:
> On Thu, Sep 1, 2022 at 3:52 AM Jan Kara <jack@xxxxxxx> wrote:
> > On Wed 31-08-22 21:47:09, Paul Moore wrote:
> > > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <sgrubb@xxxxxxxxxx> 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 <rgb@xxxxxxxxxx>
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