Message ID | fe7827974f5aec3403b07e8e70c020422126deaa.1721931241.git.josef@toxicpanda.com (mailing list archive) |
---|---|
State | New |
Headers | show |
Series | fanotify: add pre-content hooks | expand |
On Thu 25-07-24 14:19:46, Josef Bacik wrote: > From: Amir Goldstein <amir73il@gmail.com> > > With FAN_DENY response, user trying to perform the filesystem operation > gets an error with errno set to EPERM. > > It is useful for hierarchical storage management (HSM) service to be able > to deny access for reasons more diverse than EPERM, for example EAGAIN, > if HSM could retry the operation later. > > Allow fanotify groups with priority FAN_CLASSS_PRE_CONTENT to responsd > to permission events with the response value FAN_DENY_ERRNO(errno), > instead of FAN_DENY to return a custom error. > > Limit custom error to values to some errors expected on read(2)/write(2) ^^^ parse error. Perhaps: "Limit custom error values to errors expected on read..." > and open(2) of regular files. This list could be extended in the future. > Userspace can test for legitimate values of FAN_DENY_ERRNO(errno) by > writing a response to an fanotify group fd with a value of FAN_NOFD > in the fd field of the response. > > The change in fanotify_response is backward compatible, because errno is > written in the high 8 bits of the 32bit response field and old kernels > reject respose value with high bits set. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> ... > @@ -258,18 +258,25 @@ static int fanotify_get_response(struct fsnotify_group *group, > } > > /* userspace responded, convert to something usable */ > - switch (event->response & FANOTIFY_RESPONSE_ACCESS) { > + switch (FAN_RESPONSE_ACCESS(event->response)) { > case FAN_ALLOW: > ret = 0; > break; > case FAN_DENY: > + /* Check custom errno from pre-content events */ > + errno = FAN_RESPONSE_ERRNO(event->response); > + if (errno) { > + ret = -errno; > + break; > + } > + fallthrough; > default: > ret = -EPERM; > } > > /* Check if the response should be audited */ > if (event->response & FAN_AUDIT) > - audit_fanotify(event->response & ~FAN_AUDIT, > + audit_fanotify(FAN_RESPONSE_ACCESS(event->response), > &event->audit_rule); I think you need to also keep FAN_INFO in the flags not to break some userspace possibly parsing audit requests. > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index c3c8b2ea80b6..b4d810168521 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -337,11 +337,12 @@ static int process_access_response(struct fsnotify_group *group, > struct fanotify_perm_event *event; > int fd = response_struct->fd; > u32 response = response_struct->response; > + int errno = FAN_RESPONSE_ERRNO(response); > int ret = info_len; > struct fanotify_response_info_audit_rule friar; > > - pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%zu\n", __func__, > - group, fd, response, info, info_len); > + pr_debug("%s: group=%p fd=%d response=%x errno=%d buf=%p size=%zu\n", > + __func__, group, fd, response, errno, info, info_len); > /* > * 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 > @@ -350,9 +351,33 @@ static int process_access_response(struct fsnotify_group *group, > if (response & ~FANOTIFY_RESPONSE_VALID_MASK) > return -EINVAL; > > - switch (response & FANOTIFY_RESPONSE_ACCESS) { > + switch (FAN_RESPONSE_ACCESS(response)) { > case FAN_ALLOW: > + if (errno) > + return -EINVAL; > + break; > case FAN_DENY: > + /* Custom errno is supported only for pre-content groups */ > + if (errno && group->priority != FSNOTIFY_PRIO_PRE_CONTENT) > + return -EINVAL; > + > + /* > + * Limit errno to values expected on open(2)/read(2)/write(2) > + * of regular files. > + */ > + switch (errno) { > + case 0: > + case EIO: > + case EPERM: > + case EBUSY: > + case ETXTBSY: > + case EAGAIN: > + case ENOSPC: > + case EDQUOT: > + break; > + default: > + return -EINVAL; > + } > break; > default: > return -EINVAL; > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > index ae6cb2688d52..76d818a7d654 100644 > --- a/include/linux/fanotify.h > +++ b/include/linux/fanotify.h > @@ -132,7 +132,14 @@ > /* These masks check for invalid bits in permission responses. */ > #define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY) > #define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO) > -#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS) > +#define FANOTIFY_RESPONSE_ERRNO (FAN_ERRNO_MASK << FAN_ERRNO_SHIFT) > +#define FANOTIFY_RESPONSE_VALID_MASK \ > + (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS | \ > + FANOTIFY_RESPONSE_ERRNO) > + > +/* errno other than EPERM can specified in upper byte of deny response */ > +#define FAN_RESPONSE_ACCESS(res) ((res) & FANOTIFY_RESPONSE_ACCESS) > +#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> FAN_ERRNO_SHIFT)) I have to say I find the names FANOTIFY_RESPONSE_ERRNO and FAN_RESPONSE_ERRNO() (and similarly with FAN_RESPONSE_ACCESS) very similar and thus confusing. I was staring at it for 5 minutes wondering how comes it compiles before I realized one prefix is shorter than the other one so the indentifiers are indeed different. Maybe we'd make these inline functions instead of macros and name them like: fanotify_get_response_decision() fanotify_get_response_errno() ??? Honza
On Thu, Aug 1, 2024 at 11:14 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 25-07-24 14:19:46, Josef Bacik wrote: > > From: Amir Goldstein <amir73il@gmail.com> > > > > With FAN_DENY response, user trying to perform the filesystem operation > > gets an error with errno set to EPERM. > > > > It is useful for hierarchical storage management (HSM) service to be able > > to deny access for reasons more diverse than EPERM, for example EAGAIN, > > if HSM could retry the operation later. > > > > Allow fanotify groups with priority FAN_CLASSS_PRE_CONTENT to responsd > > to permission events with the response value FAN_DENY_ERRNO(errno), > > instead of FAN_DENY to return a custom error. > > > > Limit custom error to values to some errors expected on read(2)/write(2) > ^^^ parse error. Perhaps: "Limit custom error values to errors > expected on read..." > > > and open(2) of regular files. This list could be extended in the future. > > Userspace can test for legitimate values of FAN_DENY_ERRNO(errno) by > > writing a response to an fanotify group fd with a value of FAN_NOFD > > in the fd field of the response. > > > > The change in fanotify_response is backward compatible, because errno is > > written in the high 8 bits of the 32bit response field and old kernels > > reject respose value with high bits set. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > ... > > > @@ -258,18 +258,25 @@ static int fanotify_get_response(struct fsnotify_group *group, > > } > > > > /* userspace responded, convert to something usable */ > > - switch (event->response & FANOTIFY_RESPONSE_ACCESS) { > > + switch (FAN_RESPONSE_ACCESS(event->response)) { > > case FAN_ALLOW: > > ret = 0; > > break; > > case FAN_DENY: > > + /* Check custom errno from pre-content events */ > > + errno = FAN_RESPONSE_ERRNO(event->response); > > + if (errno) { > > + ret = -errno; > > + break; > > + } > > + fallthrough; > > default: > > ret = -EPERM; > > } > > > > /* Check if the response should be audited */ > > if (event->response & FAN_AUDIT) > > - audit_fanotify(event->response & ~FAN_AUDIT, > > + audit_fanotify(FAN_RESPONSE_ACCESS(event->response), > > &event->audit_rule); > > I think you need to also keep FAN_INFO in the flags not to break some > userspace possibly parsing audit requests. > > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index c3c8b2ea80b6..b4d810168521 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -337,11 +337,12 @@ static int process_access_response(struct fsnotify_group *group, > > struct fanotify_perm_event *event; > > int fd = response_struct->fd; > > u32 response = response_struct->response; > > + int errno = FAN_RESPONSE_ERRNO(response); > > int ret = info_len; > > struct fanotify_response_info_audit_rule friar; > > > > - pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%zu\n", __func__, > > - group, fd, response, info, info_len); > > + pr_debug("%s: group=%p fd=%d response=%x errno=%d buf=%p size=%zu\n", > > + __func__, group, fd, response, errno, info, info_len); > > /* > > * 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 > > @@ -350,9 +351,33 @@ static int process_access_response(struct fsnotify_group *group, > > if (response & ~FANOTIFY_RESPONSE_VALID_MASK) > > return -EINVAL; > > > > - switch (response & FANOTIFY_RESPONSE_ACCESS) { > > + switch (FAN_RESPONSE_ACCESS(response)) { > > case FAN_ALLOW: > > + if (errno) > > + return -EINVAL; > > + break; > > case FAN_DENY: > > + /* Custom errno is supported only for pre-content groups */ > > + if (errno && group->priority != FSNOTIFY_PRIO_PRE_CONTENT) > > + return -EINVAL; > > + > > + /* > > + * Limit errno to values expected on open(2)/read(2)/write(2) > > + * of regular files. > > + */ > > + switch (errno) { > > + case 0: > > + case EIO: > > + case EPERM: > > + case EBUSY: > > + case ETXTBSY: > > + case EAGAIN: > > + case ENOSPC: > > + case EDQUOT: > > + break; > > + default: > > + return -EINVAL; > > + } > > break; > > default: > > return -EINVAL; > > diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h > > index ae6cb2688d52..76d818a7d654 100644 > > --- a/include/linux/fanotify.h > > +++ b/include/linux/fanotify.h > > @@ -132,7 +132,14 @@ > > /* These masks check for invalid bits in permission responses. */ > > #define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY) > > #define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO) > > -#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS) > > +#define FANOTIFY_RESPONSE_ERRNO (FAN_ERRNO_MASK << FAN_ERRNO_SHIFT) > > +#define FANOTIFY_RESPONSE_VALID_MASK \ > > + (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS | \ > > + FANOTIFY_RESPONSE_ERRNO) > > + > > +/* errno other than EPERM can specified in upper byte of deny response */ > > +#define FAN_RESPONSE_ACCESS(res) ((res) & FANOTIFY_RESPONSE_ACCESS) > > +#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> FAN_ERRNO_SHIFT)) > > I have to say I find the names FANOTIFY_RESPONSE_ERRNO and > FAN_RESPONSE_ERRNO() (and similarly with FAN_RESPONSE_ACCESS) very similar > and thus confusing. I was staring at it for 5 minutes wondering how comes > it compiles before I realized one prefix is shorter than the other one so > the indentifiers are indeed different. Maybe we'd make these inline > functions instead of macros and name them like: > > fanotify_get_response_decision() > fanotify_get_response_errno() Sounds good to me. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 8fa439bd47d6..48539acc32e0 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -224,7 +224,7 @@ static int fanotify_get_response(struct fsnotify_group *group, struct fanotify_perm_event *event, struct fsnotify_iter_info *iter_info) { - int ret; + int ret, errno; pr_debug("%s: group=%p event=%p\n", __func__, group, event); @@ -258,18 +258,25 @@ static int fanotify_get_response(struct fsnotify_group *group, } /* userspace responded, convert to something usable */ - switch (event->response & FANOTIFY_RESPONSE_ACCESS) { + switch (FAN_RESPONSE_ACCESS(event->response)) { case FAN_ALLOW: ret = 0; break; case FAN_DENY: + /* Check custom errno from pre-content events */ + errno = FAN_RESPONSE_ERRNO(event->response); + if (errno) { + ret = -errno; + break; + } + fallthrough; default: ret = -EPERM; } /* Check if the response should be audited */ if (event->response & FAN_AUDIT) - audit_fanotify(event->response & ~FAN_AUDIT, + audit_fanotify(FAN_RESPONSE_ACCESS(event->response), &event->audit_rule); pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__, diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index c3c8b2ea80b6..b4d810168521 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -337,11 +337,12 @@ static int process_access_response(struct fsnotify_group *group, struct fanotify_perm_event *event; int fd = response_struct->fd; u32 response = response_struct->response; + int errno = FAN_RESPONSE_ERRNO(response); int ret = info_len; struct fanotify_response_info_audit_rule friar; - pr_debug("%s: group=%p fd=%d response=%u buf=%p size=%zu\n", __func__, - group, fd, response, info, info_len); + pr_debug("%s: group=%p fd=%d response=%x errno=%d buf=%p size=%zu\n", + __func__, group, fd, response, errno, info, info_len); /* * 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 @@ -350,9 +351,33 @@ static int process_access_response(struct fsnotify_group *group, if (response & ~FANOTIFY_RESPONSE_VALID_MASK) return -EINVAL; - switch (response & FANOTIFY_RESPONSE_ACCESS) { + switch (FAN_RESPONSE_ACCESS(response)) { case FAN_ALLOW: + if (errno) + return -EINVAL; + break; case FAN_DENY: + /* Custom errno is supported only for pre-content groups */ + if (errno && group->priority != FSNOTIFY_PRIO_PRE_CONTENT) + return -EINVAL; + + /* + * Limit errno to values expected on open(2)/read(2)/write(2) + * of regular files. + */ + switch (errno) { + case 0: + case EIO: + case EPERM: + case EBUSY: + case ETXTBSY: + case EAGAIN: + case ENOSPC: + case EDQUOT: + break; + default: + return -EINVAL; + } break; default: return -EINVAL; diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h index ae6cb2688d52..76d818a7d654 100644 --- a/include/linux/fanotify.h +++ b/include/linux/fanotify.h @@ -132,7 +132,14 @@ /* These masks check for invalid bits in permission responses. */ #define FANOTIFY_RESPONSE_ACCESS (FAN_ALLOW | FAN_DENY) #define FANOTIFY_RESPONSE_FLAGS (FAN_AUDIT | FAN_INFO) -#define FANOTIFY_RESPONSE_VALID_MASK (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS) +#define FANOTIFY_RESPONSE_ERRNO (FAN_ERRNO_MASK << FAN_ERRNO_SHIFT) +#define FANOTIFY_RESPONSE_VALID_MASK \ + (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS | \ + FANOTIFY_RESPONSE_ERRNO) + +/* errno other than EPERM can specified in upper byte of deny response */ +#define FAN_RESPONSE_ACCESS(res) ((res) & FANOTIFY_RESPONSE_ACCESS) +#define FAN_RESPONSE_ERRNO(res) ((int)((res) >> FAN_ERRNO_SHIFT)) /* Do not use these old uapi constants internally */ #undef FAN_ALL_CLASS_BITS diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h index 7c92d0f6bf71..2206b3ec01c9 100644 --- a/include/uapi/linux/fanotify.h +++ b/include/uapi/linux/fanotify.h @@ -233,6 +233,13 @@ struct fanotify_response_info_audit_rule { /* Legit userspace responses to a _PERM event */ #define FAN_ALLOW 0x01 #define FAN_DENY 0x02 +/* errno other than EPERM can specified in upper byte of deny response */ +#define FAN_ERRNO_BITS 8 +#define FAN_ERRNO_SHIFT (32 - FAN_ERRNO_BITS) +#define FAN_ERRNO_MASK ((1 << FAN_ERRNO_BITS) - 1) +#define FAN_DENY_ERRNO(err) \ + (FAN_DENY | ((((__u32)(err)) & FAN_ERRNO_MASK) << FAN_ERRNO_SHIFT)) + #define FAN_AUDIT 0x10 /* Bitmask to create audit record for result */ #define FAN_INFO 0x20 /* Bitmask to indicate additional information */