Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1659996830.git.rgb@redhat.com> <12063373.O9o76ZdvQC@x2>
 <Yw/efLafvmimtCDq@madcap2.tricolour.ca> <5600292.DvuYhMxLoT@x2>
 <CAHC9VhSPS7dRXLU9eV3Ne6Q7q=GPpak+=QRYLa_8Z4i-fESz8w@mail.gmail.com> <20220901075158.jqwaz3pklf3rqc6q@quack3>
In-Reply-To: <20220901075158.jqwaz3pklf3rqc6q@quack3>
From:   Paul Moore <paul@paul-moore.com>
Date:   Thu, 1 Sep 2022 14:31:01 -0400
Message-ID: <CAHC9VhStnE9vGu9h5tHnS58eyb8vm8rMN4miXpLAG6fFnidD=w@mail.gmail.com>
Subject: Re: [PATCH v4 3/4] fanotify,audit: Allow audit to use the full
 permission event response
To:     Jan Kara <jack@suse.cz>, Steve Grubb <sgrubb@redhat.com>,
        Richard Guy Briggs <rgb@redhat.com>
Cc:     Linux-Audit Mailing List <linux-audit@redhat.com>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel@vger.kernel.org, Eric Paris <eparis@parisplace.org>,
        Amir Goldstein <amir73il@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Sep 1, 2022 at 3:52 AM Jan Kara <jack@suse.cz> wrote:
> On Wed 31-08-22 21:47:09, Paul Moore wrote:
> > On Wed, Aug 31, 2022 at 7:55 PM Steve Grubb <sgrubb@redhat.com> 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