Re: [PATCH net-next v3 3/5] net: af_unix: useful handling of LSM denials on SCM_RIGHTS

[Christian Brauner]

> Right now if some LSM such as Smack denies an AF_UNIX socket peer to
> receive an SCM_RIGHTS fd, the SCM_RIGHTS fd array will be cut short at
> that point, and MSG_CTRUNC is set on return of recvmsg(). This is
> highly problematic behaviour, because it leaves the receiver
> wondering what happened. As per man page MSG_CTRUNC is supposed to
> indicate that the control buffer was sized too short, but suddenly
> a permission error might result in the exact same flag being set.
> Moreover, the receiver has no chance to determine how many fds got
> originally sent and how many were suppressed.[1]
> 
> Add a SO_RIGHTS_NOTRUNC option to UNIX sockets to enable more useful
> handling of LSM denials when receiving SCM_RIGHTS messages: instead of
> truncating the message at the first blocked fd, keep every fd slot
> and store the LSM errno in the blocked slot.
> 
> [1]: https://github.com/uapi-group/kernel-features#useful-handling-of-lsm-denials-on-scm_rights
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
>
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index 34f53dde65ce..bb1b3dee02e8 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -49,6 +49,7 @@ struct unix_sock {
>  	struct scm_stat		scm_stat;
>  	int			inq_len;
>  	bool			recvmsg_inq;
> +	bool			scm_rights_notrunc;
>  #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>  	struct sk_buff		*oob_skb;
>  #endif
> diff --git a/include/net/scm.h b/include/net/scm.h
> index c52519669349..761cda0803fb 100644
> --- a/include/net/scm.h
> +++ b/include/net/scm.h
> @@ -50,8 +50,8 @@ struct scm_cookie {
>  #endif
>  };
>  
> -void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm);
> -void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm);
> +void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm, bool notrunc);
> +void scm_detach_fds_compat(struct msghdr *msg, struct scm_cookie *scm, bool notrunc);
>  int __scm_send(struct socket *sock, struct msghdr *msg, struct scm_cookie *scm);
>  void __scm_destroy(struct scm_cookie *scm);
>  struct scm_fp_list *scm_fp_dup(struct scm_fp_list *fpl);
> @@ -108,11 +108,18 @@ void scm_recv_unix(struct socket *sock, struct msghdr *msg,
>  		   struct scm_cookie *scm, int flags);
>  
>  static inline int scm_recv_one_fd(struct file *f, int __user *ufd,
> -				  unsigned int flags)
> +				  unsigned int flags, bool notrunc)
>  {
> +	bool filtered;
> +	int error;
> +
>  	if (!ufd)
>  		return -EFAULT;
> -	return receive_fd(f, ufd, flags);
> +
> +	error = receive_fd_filtered(f, ufd, flags, &filtered);
> +	if (filtered && notrunc)
> +		return put_user(error, ufd);

This helper makes no sense to me. The boolean return argument is just
really nasty and you need an additional put_user() as well. At this
point, just drop receive_fd() and open-code it instead of using another
custom helper. Something like the completely untested:

diff --git a/include/net/scm.h b/include/net/scm.h
index 761cda0803fb..171b5ccd0b77 100644
--- a/include/net/scm.h
+++ b/include/net/scm.h
@@ -116,10 +116,22 @@ static inline int scm_recv_one_fd(struct file *f, int __user *ufd,
        if (!ufd)
                return -EFAULT;

-       error = receive_fd_filtered(f, ufd, flags, &filtered);
-       if (filtered && notrunc)
-               return put_user(error, ufd);
-       return error;
+       error = security_file_receive(file);
+       if (error)
+               return notrunc ? put_user(error, ufd) : error;
+
+       FD_PREPARE(fdf, flags, f);
+       if (fdf.err)
+               return fdf.err;
+       get_file(f);
+
+       error = put_user(fd_prepare_fd(fdf), ufd);
+       if (error)
+               return error;
+
+       __receive_sock(f);
+       return fd_publish(fdf);
 }

-- 
Christian Brauner <brauner@xxxxxxxxxx>