5:32 a.m.
On Tue 17-05-22 08:37:28, Amir Goldstein wrote:
On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs
<rgb(a)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 <sgrubb(a)redhat.com>
> Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2
> Suggested-by: Jan Kara <jack(a)suse.cz>
> Link: https://lore.kernel.org/r/20201001101219.GE17860@quack2.suse.cz
> Signed-off-by: Richard Guy Briggs <rgb(a)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 <jack(a)suse.com>
SUSE Labs, CR