On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
> On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <r...@redhat.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 <sgr...@redhat.com>
> > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> > Suggested-by: Jan Kara <j...@suse.cz>
> > Link: https://lore.kernel.org/r/20201001101219.ge17...@quack2.suse.cz
> > Signed-off-by: Richard Guy Briggs <r...@redhat.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 <j...@suse.com>
SUSE Labs, CR
--
Linux-audit mailing list
Linux-audit@redhat.com
https://listman.redhat.com/mailman/listinfo/linux-audit
