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

[Christian Brauner <brauner@kernel.org>]

> 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@xs4all.nl>

Reviewed-by: Christian Brauner (Amutable) <brauner@kernel.org>

> -void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
> +int scm_recv_one_fd(struct file *f, int __user *ufd, unsigned int flags,
> +		    bool notrunc)
> +{
> +	int error;
> +
> +	if (!ufd)
> +		return -EFAULT;
> +
> +	error = security_file_receive(f);
> +	if (error)
> +		return notrunc ? put_user(error, ufd) : error;
> +
> +	FD_PREPARE(fdf, flags, get_file(f));
> +	if (fdf.err)
> +		return fdf.err;
> +
> +	error = put_user(fd_prepare_fd(fdf), ufd);
> +	if (error)
> +		return error;
> +
> +	__receive_sock(fd_prepare_file(fdf));
> +	return fd_publish(fdf);
> +}

Seems good.

> +
> +void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm, bool notrunc)
>  {
>  	struct cmsghdr __user *cm =
>  		(__force struct cmsghdr __user *)msg->msg_control_user;
> @@ -365,12 +389,12 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>  		return;
>  
>  	if (msg->msg_flags & MSG_CMSG_COMPAT) {
> -		scm_detach_fds_compat(msg, scm);
> +		scm_detach_fds_compat(msg, scm, notrunc);
>  		return;
>  	}
>  
>  	for (i = 0; i < fdmax; i++) {
> -		err = scm_recv_one_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
> +		err = scm_recv_one_fd(scm->fp->fp[i], cmsg_data + i, o_flags, notrunc);
>  		if (err < 0)
>  			break;
>  	}
> @@ -542,8 +566,14 @@ void scm_recv_unix(struct socket *sock, struct msghdr *msg,
>  	if (!__scm_recv_common(sock->sk, msg, scm, flags))
>  		return;
>  
> -	if (scm->fp)
> -		scm_detach_fds(msg, scm);
> +	if (scm->fp) {
> +		struct unix_sock *u;
> +		bool notrunc;
> +
> +		u = unix_sk(sock->sk);
> +		notrunc = READ_ONCE(u->scm_rights_notrunc);
> +		scm_detach_fds(msg, scm, notrunc);

Minor nit: Really no need for the boolean. Would be enough to do:

scm_detach_fds(msg, scm, READ_ONCE(u->scm_rights_notrunc));

-- 
Christian Brauner <brauner@kernel.org>