On Tue 17-05-22 08:37:28, Amir Goldstein wrote: > On Mon, May 16, 2022 at 11:22 PM Richard Guy Briggs <rgb@xxxxxxxxxx> 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@xxxxxxxxxx> > > Link: https://lore.kernel.org/r/2745105.e9J7NaK4W3@x2 > > Suggested-by: Jan Kara <jack@xxxxxxx> > > Link: https://lore.kernel.org/r/20201001101219.GE17860@xxxxxxxxxxxxxx > > Signed-off-by: Richard Guy Briggs <rgb@xxxxxxxxxx> ... > > 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@xxxxxxxx> SUSE Labs, CR
