Date:   Tue, 17 May 2022 12:32:36 +0200
From:   Jan Kara <jack@...e.cz>
To:     Amir Goldstein <amir73il@...il.com>
Cc:     Richard Guy Briggs <rgb@...hat.com>,
        Linux-Audit Mailing List <linux-audit@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Paul Moore <paul@...l-moore.com>,
        Eric Paris <eparis@...isplace.org>,
        Steve Grubb <sgrubb@...hat.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v3 2/3] fanotify: define struct members to hold response
 decision context

On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <rgb@...hat.com> wrote:
> >
> > This patch adds 2 structure members to the response returned from user
> > space on a permission event. The first field is 32 bits for the context
> > type.  The context type will describe what the meaning is of the second
> > field. The default is none. The patch defines one additional context
> > type which means that the second field is a union containing a 32-bit
> > rule number. This will allow for the creation of other context types in
> > the future if other users of the API identify different needs.  The
> > second field size is defined by the context type and can be used to pass
> > along the data described by the context.
> >
> > To support this, there is a macro for user space to check that the data
> > being sent is valid. Of course, without this check, anything that
> > overflows the bit field will trigger an EINVAL based on the use of
> > FAN_INVALID_RESPONSE_MASK in process_access_response().
> >
> > Suggested-by: Steve Grubb <sgrubb@...hat.com>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <jack@...e.cz>
> > Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs <rgb@...hat.com>

...
> >  static int process_access_response(struct fsnotify_group *group,
> > -                                  struct fanotify_response *response_struct)
> > +                                  struct fanotify_response *response_struct,
> > +                                  size_t count)
> >  {
> >         struct fanotify_perm_event *event;
> >         int fd = response_struct->fd;
> >         u32 response = response_struct->response;
> >
> > -       pr_debug("%s: group=%p fd=%d response=%u\n", __func__, group,
> > -                fd, response);
> > +       pr_debug("%s: group=%p fd=%d response=%u type=%u size=%lu\n", __func__,
> > +                group, fd, response, response_struct->extra_info_type, count);
> > +       if (fd < 0)
> > +               return -EINVAL;
> >         /*
> >          * make sure the response is valid, if invalid we do nothing and either
> >          * userspace can send a valid response or we will clean it up after the
> >          * timeout
> >          */
> > -       switch (response & ~FAN_AUDIT) {
> > -       case FAN_ALLOW:
> > -       case FAN_DENY:
> > -               break;
> > -       default:
> > -               return -EINVAL;
> > -       }
> > -
> > -       if (fd < 0)
> > +       if (FAN_INVALID_RESPONSE_MASK(response))
> 
> That is a logic change, because now the response value of 0 becomes valid.
> 
> Since you did not document this change in the commit message I assume this was
> non intentional?
> 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
> 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 have to admit I didn't quite grok your suggestion here although I
understand (and agree with) the general direction of the proposal :). Maybe
code would explain it better what you have in mind?

> > +/*
> > + * User space may need to record additional information about its decision.
> > + * The extra information type records what kind of information is included.
> > + * The default is none. We also define an extra informaion buffer whose
> 
> typo: informaion
> 
> > + * 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.
> 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.

Yes, please no unions in the API, that is always painful with the
alignment, size etc. What I had in mind was:

Keep fanotify_response as is:

struct fanotify_response {
	__s32 fd;
	__u32 response;
};

Define extra info header:

struct fanotify_response_info_header {
	__u8 info_type;
	__u8 pad;
	__u16 len;
};

And then struct for your audit rule:

struct fanotify_response_info_audit_rule {
	struct fanotify_response_info_header hdr;
	__u32 audit_rule;
};

The verification in fanotify_write() then goes like:

	struct fanotify_response response;
	char extra_info_buf[sizeof(struct fanotify_response_info_audit_rule)];

	if (copy_from_user(&response, buf, sizeof(response)))
		return -EFAULT;

	if (!(response.response & FAN_EXTRA_INFO)) {
		count = 0;
	} else {
		count -= sizeof(response);

		/* Simplistic parsing for now */
		if (count != sizeof(struct fanotify_response_info_audit_rule))
			return -EINVAL;
		if (copy_from_user(extra_info_buf, buf, count)
			return -EFAULT;
	}

	ret = process_access_response(group, &response, extra_info_buf, count);

And we pass extra_info_buf and count to audit_fanotify() where we need to do
further validation like: 

	struct fanotify_response_info_audit_rule *audit_response = NULL;

	if (count > 0) {
		/* Just one possible info type for now */
		audit_response = (struct fanotify_response_info_audit_rule *)extra_info_buf;
		if (audit_response->info_type != FAN_RESPONSE_INFO_AUDIT_RULE)
			return -EINVAL;
		if (audit_response->pad != 0)
			return -EINVAL;
		if (audit_response->len != sizeof(*audit_response))
			return -EINVAL;
	}

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists