Message-ID: <YnGX/6BmTV6X5v1q@madcap2.tricolour.ca>
Date:   Tue, 3 May 2022 17:00:47 -0400
From:   Richard Guy Briggs <rgb@...hat.com>
To:     Paul Moore <paul@...l-moore.com>
Cc:     Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org, Eric Paris <eparis@...isplace.org>,
        Steve Grubb <sgrubb@...hat.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v2 0/3] fanotify: Allow user space to pass back
 additional audit info

On 2022-05-02 20:16, Paul Moore wrote:
> On Thu, Apr 28, 2022 at 8:55 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> > On 2022-04-28 20:44, 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 patch defines 2 additional fields within the response structure
> > > returned from user space on a permission event. The first field is 16
> > > bits for the context type. The context type will describe what the
> > > meaning is of the second field. The audit system will separate the
> > > pieces and log them individually.
> > >
> > > 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_ctx=17
> >
> > It might have been a good idea to tag this as RFC...  I have a few
> > questions:
> >
> > 1. Where did "resp=" come from?
> 
> According to the git log, it came from Steve Grubb via de8cd83e91bc
> ("audit: Record fanotify access control decisions").  Steve should
> have known what he was doing with respect to field names so I'm
> assuming he had a reason.
> 
> > It isn't in the field dictionary.  It
> > seems like a needless duplication of "res=".  If it isn't, maybe it
> > should have a "fan_" namespace prefix and become "fan_res="?
> 
> Regardless of what it should have been, it is "resp" now and we can't
> really change it.  As far as the field dictionary is concerned, while
> we should document these fields, it is important to note that when the
> dictionary conflicts with the kernel, the kernel wins by definition.

Agree on all counts.  It was an open-ended question.  It is also moot
since it is even expected in the audit-testsuite and would break that if
it were changed.

> > 2. It appears I'm ok changing the "__u32 response" to "__u16" without
> > breaking old userspace.  Is this true on all arches?
> 
> I can't answer that for you, the fanotify folks will need to look at
> that, but you likely already know that.  While I haven't gone through
> the entire patchset yet, if it was me I probably would have left
> response as a u32 and just added the extra fields; you are already
> increasing the size of fanotify_response so why bother with shrinking
> an existing field?

I was thinking of that, but chose to follow the lead of the fanotify
mainainer.

> > 3. What should be the action if response contains unknown flags or
> > types?  Is it reasonable to return -EINVAL?
> 
> Once again, not a fanotify expert, but EINVAL is intended for invalid
> input so it seems like a reasonable choice.

The choice of the error code wasn't in question but rather the need to
fail rather than ignore unknown flags.

> > 4. Currently, struct fanotify_response has a fixed size, but if future
> > types get defined that have variable buffer sizes, how would that be
> > communicated or encoded?
> 
> If that is a concern, you should probably include a length field in
> the structure before the variable length field.  You can't put it
> before fd or response, so it's really a question of before or after
> your new extra_info_type; I might suggest *after* extra_info_type, but
> that's just me.

After extra_info_type is what I was thinking.  The other possibility is
that a type with a variable length field could define its data size as
the first field within the variable field as set out in the format of
that varible length field so that all the fixed length fields would not
need to waste that space or bandwidth.

Thanks for the feedback.

> paul-moore.com

- RGB

--
Richard Guy Briggs <rgb@...hat.com>
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

Powered by blists - more mailing lists