From: Amir Goldstein <amir73il@gmail.com>
To: Richard Guy Briggs <rgb@redhat.com>
Cc: Linux-Audit Mailing List <linux-audit@redhat.com>,
LKML <linux-kernel@vger.kernel.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
Paul Moore <paul@paul-moore.com>,
Eric Paris <eparis@parisplace.org>,
Steve Grubb <sgrubb@redhat.com>, Jan Kara <jack@suse.cz>
Subject: Re: [PATCH v3 2/3] fanotify: define struct members to hold response decision context
Date: Thu, 19 May 2022 09:03:51 +0300 [thread overview]
Message-ID: <CAOQ4uxi+8HUqyGxQBNMqSong92nreOWLKdy9MCrYg8wgW9Dj4g@mail.gmail.com> (raw)
In-Reply-To: <YoWKPcsySt9cJbtB@madcap2.tricolour.ca>
> > However, this behavior change is something that I did ask for, but it should be
> > done is a separate commit:
> >
> > /* These are NOT bitwise flags. Both bits can be used together. */
> > #define FAN_TEST 0x00
> > #define FAN_ALLOW 0x01
> > #define FAN_DENY 0x02
> > #define FANOTIFY_RESPONSE_ACCESS \
> > (FAN_TEST|FAN_ALLOW | FAN_DENY)
> >
> > ...
> > int access = response & FANOTIFY_RESPONSE_ACCESS;
> >
> > 1. Do return EINVAL for access == 0
>
> Going back to the original code will do that.
Oops, this was supposed to be Do NOT return EINVAL for access == 0
this is the case of FAN_TEST.
The patch I posted later explains that better.
>
> > 2. Let all the rest of the EINVAL checks run (including extra type)
> > 3. Move if (fd < 0) to last check
> > 4. Add if (!access) return 0 before if (fd < 0)
> >
> > That will provide a mechanism for userspace to probe the
> > kernel support for extra types in general and specific types
> > that it may respond with.
>
> I'm still resisting the idea of the TEST flag... It seems like an
> unneeded extra step and complication...
Please reply to the patch I posted as a reply as point
at said complication. There is no extra step.
>
> The simple presence of the FAN_EXTRA flag should sort it out and could
> even make TEST one of the types.
>
I think you've missed the point of the TEST response code.
The point of the TEST response code is to test whether the
extra type is supported, so TESTS cannot be a type.
You should not think of FAN_TEST as a flag at all, in
fact, it is semantic and can be omitted altogether.
The core of the idea is that:
int access = response & FANOTIFY_RESPONSE_ACCESS;
access is an enum, not a bitwise mask, much like:
unsigned int class = flags & FANOTIFY_CLASS_BITS;
unsigned int mark_type = flags & FANOTIFY_MARK_TYPE_BITS;
At the moment, userspace must provide a valid access code
either ALLOW or DENY.
Providing no access code (0) is not valid.
I suggest making FAN_EXTRA with no access code a valid
response for testing the EXTRA types support.
(please refer to the patch)
[...]
> > > + * size is determined by the extra information type.
> > > + *
> > > + * If the context type is Rule, then the context following is the rule number
> > > + * that triggered the user space decision.
> > > + */
> > > +
> > > +#define FAN_RESPONSE_INFO_NONE 0
> > > +#define FAN_RESPONSE_INFO_AUDIT_RULE 1
> > > +
> > > +union fanotify_response_extra {
> > > + __u32 audit_rule;
> > > +};
> > > +
> > > struct fanotify_response {
> > > __s32 fd;
> > > __u32 response;
> > > + __u32 extra_info_type;
> > > + union fanotify_response_extra extra_info;
> >
> > IIRC, Jan wanted this to be a variable size record with info_type and info_len.
>
> Again, the intent was to make it fixed for now and change it later if
> needed, but that was a shortsighted approach...
>
> I don't see a need for a len in all response types. _NONE doesn't need
> any. _AUDIT_RULE is known to be 32 bits. Other types can define their
> size and layout as needed, including a len field if it is needed.
>
len is part of a common response info header.
It is meant to make writing generic code.
So Jan's email.
> > I don't know if we want to make this flexible enough to allow for multiple
> > records in the future like we do in events, but the common wisdom of
> > the universe says that if we don't do it, we will need it.
>
> It did occur to me that this could be used for other than audit, hence
> the renaming of the ..."_NONE" macro.
>
> We should be able in the future to define a type that is extensible or
> has multiple records. We have (2^32) - 2 types left to work with.
>
The way this was done when we first introduced event info
records was the same. We only allowed one type of record
and a single record to begin with, but the format allowed for
extending to multiple records.
struct fanotify_event_metadata already had event_len and
metadata_len, so that was convenient. Supporting multi
records only required that every record has a header with its
own len.
As far as I can tell, the case of fanotify_response is different
because we have the count argument of write(), which serves
as the total response_len.
If we ever want to be able to extend the base fanotify_response,
add fields to it not as extra info records, then we need to add
response_metadata_len to struct fanotify_response, but I think that
would be over design.
Thanks,
Amir.
next prev parent reply other threads:[~2022-05-19 6:04 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-16 20:22 [PATCH v3 0/3] fanotify: Allow user space to pass back additional audit info Richard Guy Briggs
2022-05-16 20:22 ` [PATCH v3 1/3] fanotify: Ensure consistent variable type for response Richard Guy Briggs
2022-05-16 23:06 ` Paul Moore
2022-05-16 20:22 ` [PATCH v3 2/3] fanotify: define struct members to hold response decision context Richard Guy Briggs
2022-05-17 5:37 ` Amir Goldstein
2022-05-17 10:32 ` Jan Kara
2022-05-17 11:31 ` Amir Goldstein
2022-05-17 12:06 ` Amir Goldstein
2022-05-19 0:07 ` Richard Guy Briggs
2022-05-19 6:03 ` Amir Goldstein [this message]
2022-05-19 9:55 ` Jan Kara
2022-05-17 7:16 ` kernel test robot
2022-05-17 7:26 ` kernel test robot
2022-05-16 20:22 ` [PATCH v3 3/3] fanotify: Allow audit to use the full permission event response Richard Guy Briggs
2022-05-17 1:42 ` Paul Moore
2022-05-17 1:57 ` Richard Guy Briggs
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxi+8HUqyGxQBNMqSong92nreOWLKdy9MCrYg8wgW9Dj4g@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=eparis@parisplace.org \
--cc=jack@suse.cz \
--cc=linux-audit@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paul@paul-moore.com \
--cc=rgb@redhat.com \
--cc=sgrubb@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).