On Thu, Aug 1, 2024 at 11:14 PM Jan Kara <jack@xxxxxxx> wrote:
>
> On Thu 25-07-24 14:19:46, Josef Bacik wrote:
> > From: Amir Goldstein <amir73il@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
>
> ...
>
> > @@ -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.
