Re: [PATCH net v3 2/3] net: prevent rewrite of msg_name in sock_sendmsg()

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Jordan Rife wrote:
> Callers of sock_sendmsg(), and similarly kernel_sendmsg(), in kernel
> space may observe their value of msg_name change in cases where BPF
> sendmsg hooks rewrite the send address. This has been confirmed to break
> NFS mounts running in UDP mode and has the potential to break other
> systems.
> 
> This patch:
> 
> 1) Creates a new function called __sock_sendmsg() with same logic as the
>    old sock_sendmsg() function.
> 2) Replaces calls to sock_sendmsg() made by __sys_sendto() and
>    __sys_sendmsg() with __sock_sendmsg() to avoid an unnecessary copy,
>    as these system calls are already protected.
> 3) Modifies sock_sendmsg() so that it makes a copy of msg_name if
>    present before passing it down the stack to insulate callers from
>    changes to the send address.
> 
> Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
> Fixes: 1cedee13d25a ("bpf: Hooks for sys_sendmsg")
> Signed-off-by: Jordan Rife <jrife@google.com>
> ---
> v2->v3: Add "Fixes" tag.
> v1->v2: Split up original patch into patch series. Perform address copy
> 	in sock_sendmsg() instead of sock->ops->sendmsg().
> 
>  net/socket.c | 32 ++++++++++++++++++++++++++------
>  1 file changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index eb7f14143caed..2d34a69b84406 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -737,6 +737,14 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg)
>  	return ret;
>  }
>  
> +static int __sock_sendmsg(struct socket *sock, struct msghdr *msg)
> +{
> +	int err = security_socket_sendmsg(sock, msg,
> +					  msg_data_left(msg));
> +
> +	return err ?: sock_sendmsg_nosec(sock, msg);
> +}
> +
>  /**
>   *	sock_sendmsg - send a message through @sock
>   *	@sock: socket
> @@ -747,10 +755,22 @@ static inline int sock_sendmsg_nosec(struct socket *sock, struct msghdr *msg)
>   */
>  int sock_sendmsg(struct socket *sock, struct msghdr *msg)
>  {
> -	int err = security_socket_sendmsg(sock, msg,
> -					  msg_data_left(msg));
> +	struct sockaddr_storage address;
> +	struct sockaddr_storage *save_addr = (struct sockaddr_storage *)msg->msg_name;

Since there's feedback on patch 3/3: please maintain reverse xmas tree:
reorder these two declarations from longest to shortest.
> +	int ret;
>  
> -	return err ?: sock_sendmsg_nosec(sock, msg);
> +	if (msg->msg_name) {
> +		if (msg->msg_namelen < 0 || msg->msg_namelen > sizeof(address))
> +			return -EINVAL;
> +
> +		memcpy(&address, msg->msg_name, msg->msg_namelen);
> +		msg->msg_name = &address;
> +	}
> +
> +	ret = __sock_sendmsg(sock, msg);
> +	msg->msg_name = save_addr;
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL(sock_sendmsg);
>  
> @@ -1138,7 +1158,7 @@ static ssize_t sock_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	if (sock->type == SOCK_SEQPACKET)
>  		msg.msg_flags |= MSG_EOR;
>  
> -	res = sock_sendmsg(sock, &msg);
> +	res = __sock_sendmsg(sock, &msg);
>  	*from = msg.msg_iter;
>  	return res;
>  }
> @@ -2174,7 +2194,7 @@ int __sys_sendto(int fd, void __user *buff, size_t len, unsigned int flags,
>  	if (sock->file->f_flags & O_NONBLOCK)
>  		flags |= MSG_DONTWAIT;
>  	msg.msg_flags = flags;
> -	err = sock_sendmsg(sock, &msg);
> +	err = __sock_sendmsg(sock, &msg);
>  
>  out_put:
>  	fput_light(sock->file, fput_needed);
> @@ -2538,7 +2558,7 @@ static int ____sys_sendmsg(struct socket *sock, struct msghdr *msg_sys,
>  		err = sock_sendmsg_nosec(sock, msg_sys);
>  		goto out_freectl;
>  	}
> -	err = sock_sendmsg(sock, msg_sys);
> +	err = __sock_sendmsg(sock, msg_sys);
>  	/*
>  	 * If this is sendmmsg() and sending to current destination address was
>  	 * successful, remember it.
> -- 
> 2.42.0.459.ge4e396fd5e-goog
>