Re: [PATCH v3 for-next 2/3] net: copy from user before calling __get_compat_msghdr

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi,

On 14.07.2022 13:02, Dylan Yudaken wrote:
> this is in preparation for multishot receive from io_uring, where it needs
> to have access to the original struct user_msghdr.
>
> functionally this should be a no-op.
>
> Acked-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Dylan Yudaken <dylany@fb.com>

This patch landed in linux next-20220715 as commit 1a3e4e94a1b9 ("net: 
copy from user before calling __get_compat_msghdr"). Unfortunately it 
causes a serious regression on the ARM64 based Khadas VIM3l board:

Unable to handle kernel access to user memory outside uaccess routines 
at virtual address 00000000ffc4a5c8
Mem abort info:
   ESR = 0x000000009600000f
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x0f: level 3 permission fault
Data abort info:
   ISV = 0, ISS = 0x0000000f
   CM = 0, WnR = 0
user pgtable: 4k pages, 48-bit VAs, pgdp=0000000001909000
[00000000ffc4a5c8] pgd=0800000001a7b003, p4d=0800000001a7b003, 
pud=0800000001a0e003, pmd=0800000001913003, pte=00e800000b9baf43
Internal error: Oops: 9600000f [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 247 Comm: systemd-udevd Not tainted 5.19.0-rc6+ #12437
Hardware name: Khadas VIM3L (DT)
pstate: 80400009 (Nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : get_compat_msghdr+0xd0/0x1b0
lr : get_compat_msghdr+0xcc/0x1b0
...
Call trace:
  get_compat_msghdr+0xd0/0x1b0
  ___sys_sendmsg+0xd0/0xe0
  __sys_sendmsg+0x68/0xc4
  __arm64_compat_sys_sendmsg+0x28/0x3c
  invoke_syscall+0x48/0x114
  el0_svc_common.constprop.0+0x60/0x11c
  do_el0_svc_compat+0x1c/0x50
  el0_svc_compat+0x58/0x100
  el0t_32_sync_handler+0x90/0x140
  el0t_32_sync+0x190/0x194
Code: d2800382 9100f3e0 97d9be02 b5fffd60 (b9401a60)
---[ end trace 0000000000000000 ]---

This happens only on the mentioned board, other my ARM64 test boards 
boot fine with next-20220715. Reverting this commit, together with 
2b0b67d55f13 ("fix up for "io_uring: support multishot in recvmsg"") and 
a8b38c4ce724 ("io_uring: support multishot in recvmsg") due to compile 
dependencies on top of next-20220715 fixes the issue.

Let me know how I can help fixing this issue.

> ---
>   include/net/compat.h |  5 ++---
>   io_uring/net.c       | 17 +++++++++--------
>   net/compat.c         | 39 +++++++++++++++++----------------------
>   3 files changed, 28 insertions(+), 33 deletions(-)
>
> diff --git a/include/net/compat.h b/include/net/compat.h
> index 595fee069b82..84c163f40f38 100644
> --- a/include/net/compat.h
> +++ b/include/net/compat.h
> @@ -46,9 +46,8 @@ struct compat_rtentry {
>   	unsigned short  rt_irtt;        /* Initial RTT                  */
>   };
>   
> -int __get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg,
> -			struct sockaddr __user **save_addr, compat_uptr_t *ptr,
> -			compat_size_t *len);
> +int __get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr *msg,
> +			struct sockaddr __user **save_addr);
>   int get_compat_msghdr(struct msghdr *, struct compat_msghdr __user *,
>   		      struct sockaddr __user **, struct iovec **);
>   int put_cmsg_compat(struct msghdr*, int, int, int, void *);
> diff --git a/io_uring/net.c b/io_uring/net.c
> index da7667ed3610..5bc3440a8290 100644
> --- a/io_uring/net.c
> +++ b/io_uring/net.c
> @@ -369,24 +369,25 @@ static int __io_compat_recvmsg_copy_hdr(struct io_kiocb *req,
>   					struct io_async_msghdr *iomsg)
>   {
>   	struct io_sr_msg *sr = io_kiocb_to_cmd(req);
> +	struct compat_msghdr msg;
>   	struct compat_iovec __user *uiov;
> -	compat_uptr_t ptr;
> -	compat_size_t len;
>   	int ret;
>   
> -	ret = __get_compat_msghdr(&iomsg->msg, sr->umsg_compat, &iomsg->uaddr,
> -				  &ptr, &len);
> +	if (copy_from_user(&msg, sr->umsg_compat, sizeof(msg)))
> +		return -EFAULT;
> +
> +	ret = __get_compat_msghdr(&iomsg->msg, sr->umsg_compat, &iomsg->uaddr);
>   	if (ret)
>   		return ret;
>   
> -	uiov = compat_ptr(ptr);
> +	uiov = compat_ptr(msg.msg_iov);
>   	if (req->flags & REQ_F_BUFFER_SELECT) {
>   		compat_ssize_t clen;
>   
> -		if (len == 0) {
> +		if (msg.msg_iovlen == 0) {
>   			sr->len = 0;
>   			iomsg->free_iov = NULL;
> -		} else if (len > 1) {
> +		} else if (msg.msg_iovlen > 1) {
>   			return -EINVAL;
>   		} else {
>   			if (!access_ok(uiov, sizeof(*uiov)))
> @@ -400,7 +401,7 @@ static int __io_compat_recvmsg_copy_hdr(struct io_kiocb *req,
>   		}
>   	} else {
>   		iomsg->free_iov = iomsg->fast_iov;
> -		ret = __import_iovec(READ, (struct iovec __user *)uiov, len,
> +		ret = __import_iovec(READ, (struct iovec __user *)uiov, msg.msg_iovlen,
>   				   UIO_FASTIOV, &iomsg->free_iov,
>   				   &iomsg->msg.msg_iter, true);
>   		if (ret < 0)
> diff --git a/net/compat.c b/net/compat.c
> index 210fc3b4d0d8..513aa9a3fc64 100644
> --- a/net/compat.c
> +++ b/net/compat.c
> @@ -34,20 +34,15 @@
>   #include <net/compat.h>
>   
>   int __get_compat_msghdr(struct msghdr *kmsg,
> -			struct compat_msghdr __user *umsg,
> -			struct sockaddr __user **save_addr,
> -			compat_uptr_t *ptr, compat_size_t *len)
> +			struct compat_msghdr *msg,
> +			struct sockaddr __user **save_addr)
>   {
> -	struct compat_msghdr msg;
>   	ssize_t err;
>   
> -	if (copy_from_user(&msg, umsg, sizeof(*umsg)))
> -		return -EFAULT;
> -
> -	kmsg->msg_flags = msg.msg_flags;
> -	kmsg->msg_namelen = msg.msg_namelen;
> +	kmsg->msg_flags = msg->msg_flags;
> +	kmsg->msg_namelen = msg->msg_namelen;
>   
> -	if (!msg.msg_name)
> +	if (!msg->msg_name)
>   		kmsg->msg_namelen = 0;
>   
>   	if (kmsg->msg_namelen < 0)
> @@ -57,15 +52,15 @@ int __get_compat_msghdr(struct msghdr *kmsg,
>   		kmsg->msg_namelen = sizeof(struct sockaddr_storage);
>   
>   	kmsg->msg_control_is_user = true;
> -	kmsg->msg_control_user = compat_ptr(msg.msg_control);
> -	kmsg->msg_controllen = msg.msg_controllen;
> +	kmsg->msg_control_user = compat_ptr(msg->msg_control);
> +	kmsg->msg_controllen = msg->msg_controllen;
>   
>   	if (save_addr)
> -		*save_addr = compat_ptr(msg.msg_name);
> +		*save_addr = compat_ptr(msg->msg_name);
>   
> -	if (msg.msg_name && kmsg->msg_namelen) {
> +	if (msg->msg_name && kmsg->msg_namelen) {
>   		if (!save_addr) {
> -			err = move_addr_to_kernel(compat_ptr(msg.msg_name),
> +			err = move_addr_to_kernel(compat_ptr(msg->msg_name),
>   						  kmsg->msg_namelen,
>   						  kmsg->msg_name);
>   			if (err < 0)
> @@ -76,12 +71,10 @@ int __get_compat_msghdr(struct msghdr *kmsg,
>   		kmsg->msg_namelen = 0;
>   	}
>   
> -	if (msg.msg_iovlen > UIO_MAXIOV)
> +	if (msg->msg_iovlen > UIO_MAXIOV)
>   		return -EMSGSIZE;
>   
>   	kmsg->msg_iocb = NULL;
> -	*ptr = msg.msg_iov;
> -	*len = msg.msg_iovlen;
>   	return 0;
>   }
>   
> @@ -90,15 +83,17 @@ int get_compat_msghdr(struct msghdr *kmsg,
>   		      struct sockaddr __user **save_addr,
>   		      struct iovec **iov)
>   {
> -	compat_uptr_t ptr;
> -	compat_size_t len;
> +	struct compat_msghdr msg;
>   	ssize_t err;
>   
> -	err = __get_compat_msghdr(kmsg, umsg, save_addr, &ptr, &len);
> +	if (copy_from_user(&msg, umsg, sizeof(*umsg)))
> +		return -EFAULT;
> +
> +	err = __get_compat_msghdr(kmsg, umsg, save_addr);
>   	if (err)
>   		return err;
>   
> -	err = import_iovec(save_addr ? READ : WRITE, compat_ptr(ptr), len,
> +	err = import_iovec(save_addr ? READ : WRITE, compat_ptr(msg.msg_iov), msg.msg_iovlen,
>   			   UIO_FASTIOV, iov, &kmsg->msg_iter);
>   	return err < 0 ? err : 0;
>   }

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland