Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
MIME-Version: 1.0
References: <cover.1651174324.git.rgb@redhat.com> <Yms3hVYSRD1zT+Rz@madcap2.tricolour.ca>
In-Reply-To: <Yms3hVYSRD1zT+Rz@madcap2.tricolour.ca>
From:   Paul Moore <paul@paul-moore.com>
Date:   Mon, 2 May 2022 20:16:01 -0400
Message-ID: <CAHC9VhSGda5NofudLtsKspPjGc9bnZd=DZL9Mo-PJtJbb9RO4w@mail.gmail.com>
Subject: Re: [PATCH v2 0/3] fanotify: Allow user space to pass back additional
 audit info
To:     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>,
        Steve Grubb <sgrubb@redhat.com>, Jan Kara <jack@suse.cz>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Apr 28, 2022 at 8:55 PM Richard Guy Briggs <rgb@redhat.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.

> 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?

> 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.

> 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.


--
paul-moore.com