Re: [PATCH v2 09/16] fanotify: allow to set errno in FAN_DENY permission response

[Christian Brauner <brauner@kernel.org>]

On Thu, Aug 08, 2024 at 03:27:11PM GMT, 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 values to errors expected on read(2)/write(2) 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>
> ---
>  fs/notify/fanotify/fanotify.c      | 18 ++++++++++-----
>  fs/notify/fanotify/fanotify.h      | 10 +++++++++
>  fs/notify/fanotify/fanotify_user.c | 36 +++++++++++++++++++++++++-----
>  include/linux/fanotify.h           |  5 ++++-
>  include/uapi/linux/fanotify.h      |  7 ++++++
>  5 files changed, 65 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 4e8dce39fa8f..1cbf41b34080 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -224,7 +224,8 @@ 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;
> +	u32 decision;
>  
>  	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>  
> @@ -257,20 +258,27 @@ static int fanotify_get_response(struct fsnotify_group *group,
>  		goto out;
>  	}
>  
> +	decision = fanotify_get_response_decision(event->response);
>  	/* userspace responded, convert to something usable */
> -	switch (event->response & FANOTIFY_RESPONSE_ACCESS) {
> +	switch (decision & FANOTIFY_RESPONSE_ACCESS) {
>  	case FAN_ALLOW:
>  		ret = 0;
>  		break;
>  	case FAN_DENY:
> +		/* Check custom errno from pre-content events */
> +		errno = fanotify_get_response_errno(event->response);

Fwiw, you're fetching from event->response again but have already
stashed it in @decision earlier. Probably just an oversight.

> +		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,
> -			       &event->audit_rule);
> +	if (decision & FAN_AUDIT)
> +		audit_fanotify(decision & ~FAN_AUDIT, &event->audit_rule);
>  
>  	pr_debug("%s: group=%p event=%p about to return ret=%d\n", __func__,
>  		 group, event, ret);
> diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h
> index 7f06355afa1f..d0722ef13138 100644
> --- a/fs/notify/fanotify/fanotify.h
> +++ b/fs/notify/fanotify/fanotify.h
> @@ -528,3 +528,13 @@ static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark)
>  
>  	return mflags;
>  }
> +
> +static inline u32 fanotify_get_response_decision(u32 res)
> +{
> +	return res & (FANOTIFY_RESPONSE_ACCESS | FANOTIFY_RESPONSE_FLAGS);
> +}
> +
> +static inline int fanotify_get_response_errno(int res)
> +{
> +	return res >> FAN_ERRNO_SHIFT;
> +}
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index ed56fe6f5ec7..0a37f1c761aa 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -337,11 +337,13 @@ static int process_access_response(struct fsnotify_group *group,
>  	struct fanotify_perm_event *event;
>  	int fd = response_struct->fd;
>  	u32 response = response_struct->response;
> +	u32 decision = fanotify_get_response_decision(response);
> +	int errno = fanotify_get_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,18 +352,42 @@ static int process_access_response(struct fsnotify_group *group,
>  	if (response & ~FANOTIFY_RESPONSE_VALID_MASK)
>  		return -EINVAL;
>  
> -	switch (response & FANOTIFY_RESPONSE_ACCESS) {
> +	switch (decision & FANOTIFY_RESPONSE_ACCESS) {
>  	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;
>  	}
>  
> -	if ((response & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
> +	if ((decision & FAN_AUDIT) && !FAN_GROUP_FLAG(group, FAN_ENABLE_AUDIT))
>  		return -EINVAL;
>  
> -	if (response & FAN_INFO) {
> +	if (decision & FAN_INFO) {
>  		ret = process_access_response_info(info, info_len, &friar);
>  		if (ret < 0)
>  			return ret;
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index ae6cb2688d52..547514542669 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -132,7 +132,10 @@
>  /* 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)
>  
>  /* 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 cc28dce5f744..7b746c5fcbd8 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 */
>  
> -- 
> 2.43.0
>