Re: [PATCH v7 0/3] fanotify: Allow user space to pass back additional audit info

[Richard Guy Briggs]

On 2023-02-08 11:24, Paul Moore wrote:
> On Wed, Feb 8, 2023 at 10:27 AM Steve Grubb <sgrubb@xxxxxxxxxx> wrote:
> > On Wednesday, February 8, 2023 10:03:24 AM EST Paul Moore wrote:
> > > On Wed, Feb 8, 2023 at 7:08 AM Jan Kara <jack@xxxxxxx> wrote:
> > > > On Tue 07-02-23 09:54:11, Paul Moore wrote:
> > > > > On Tue, Feb 7, 2023 at 7:09 AM Jan Kara <jack@xxxxxxx> wrote:
> > > > > > On Fri 03-02-23 16:35:13, Richard Guy Briggs wrote:
> > > > > > > The Fanotify API can be used for access control by requesting
> > > > > > > permission
> > > > > > > event notification. The user space tooling that uses it may have a
> > > > > > > complicated policy that inherently contains additional context for
> > > > > > > the
> > > > > > > decision. If this information were available in the audit trail,
> > > > > > > policy
> > > > > > > writers can close the loop on debugging policy. Also, if this
> > > > > > > additional
> > > > > > > information were available, it would enable the creation of tools
> > > > > > > that
> > > > > > > can suggest changes to the policy similar to how audit2allow can
> > > > > > > help
> > > > > > > refine labeled security.
> > > > > > >
> > > > > > > This patchset defines a new flag (FAN_INFO) and new extensions that
> > > > > > > define additional information which are appended after the response
> > > > > > > structure returned from user space on a permission event.  The
> > > > > > > appended
> > > > > > > information is organized with headers containing a type and size
> > > > > > > that
> > > > > > > can be delegated to interested subsystems.  One new information
> > > > > > > type is
> > > > > > > defined to audit the triggering rule number.
> > > > > > >
> > > > > > > A newer kernel will work with an older userspace and an older
> > > > > > > kernel
> > > > > > > will behave as expected and reject a newer userspace, leaving it up
> > > > > > > to
> > > > > > > the newer userspace to test appropriately and adapt as necessary.
> > > > > > > This
> > > > > > > is done by providing a a fully-formed FAN_INFO extension but
> > > > > > > setting the
> > > > > > > fd to FAN_NOFD.  On a capable kernel, it will succeed but issue no
> > > > > > > audit
> > > > > > > record, whereas on an older kernel it will fail.
> > > > > > >
> > > > > > > The audit function was updated to log the additional information in
> > > > > > > the
> > > > > > > AUDIT_FANOTIFY record. The following are examples of the new record
> > > > > > >
> > > > > > > format:
> > > > > > >   type=FANOTIFY msg=audit(1600385147.372:590): resp=2 fan_type=1
> > > > > > >   fan_info=3137 subj_trust=3 obj_trust=5 type=FANOTIFY
> > > > > > >   msg=audit(1659730979.839:284): resp=1 fan_type=0 fan_info=0
> > > > > > >   subj_trust=2 obj_trust=2> > >
> > > > > > Thanks! I've applied this series to my tree.
> > > > >
> > > > > While I think this version of the patchset is fine, for future
> > > > > reference it would have been nice if you had waited for my ACK on
> > > > > patch 3/3; while Steve maintains his userspace tools, I'm the one
> > > > > responsible for maintaining the Linux Kernel's audit subsystem.
> > > >
> > > > Aha, I'm sorry for that. I had the impression that on the last version of
> > > > the series you've said you don't see anything for which the series should
> > > > be respun so once Steve's objections where addressed and you were silent
> > > > for a few days, I thought you consider the thing settled... My bad.
> > >
> > > That's understandable, especially given inconsistencies across
> > > subsystems.  If it helps, if I'm going to ACK something I make it
> > > explicit with a proper 'Acked-by: ...' line in my reply; if I say
> > > something looks good but there is no explicit ACK, there is usually
> > > something outstanding that needs to be resolved, e.g. questions,
> > > additional testing, etc.
> > >
> > > In this particular case I posed some questions in that thread and
> > > never saw a reply with any answers, hence the lack of an ACK.  While I
> > > think the patches were reasonable, I withheld my ACK until the
> > > questions were answered ... which they never were from what I can
> > > tell, we just saw a new patchset with changes.
> > >
> > > /me shrugs
> >
> > Paul,
> >
> > I reread the thread. You only had a request to change if/else to a switch
> > construct only if there was a respin for the 3F. You otherwise said get
> > Steve's input and the 3F borders on being overly clever. Both were addressed.
> > If you had other questions that needed answers on, please restate them to
> > expedite approval of this set of patches. As far as I can tell, all comments
> > are addressed.
> 
> Steve,
> 
> It might be helpful to reread my reply below:
> 
> https://lore.kernel.org/linux-audit/CAHC9VhRWDD6Tk6AEmgoobBkcVKRYbVOte7-F0TGJD2dRk7NKxw@xxxxxxxxxxxxxx/
> 
> You'll see that I made a comment in that email about not following
> Richard's explanation about "encoding the zero" (the patch was
> encoding a "?" to the best I could tell).  I was hoping for some
> clarification from Richard on his comments, and I never saw anything
> in my inbox.  I just checked the archives on lore and I don't see
> anything there either.

Well, it could have been any of:
	?
	"?"
	3F
	30
	0

I can't answer that.  My preference is for 3F but good arguments can be
made for any of these.  I defer to Steve since it is his tools and
customers that have to deal with it.

> 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