| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <46439555-644d-08a1-7d66-16f8f9a320f0@samsung.com>
Date: Fri, 15 Jul 2022 22:28:58 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: Dylan Yudaken <dylany@...com>, Jens Axboe <axboe@...nel.dk>,
Pavel Begunkov <asml.silence@...il.com>, davem@...emloft.net,
edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
io-uring@...r.kernel.org
Cc: netdev@...r.kernel.org, Kernel-team@...com
Subject: Re: [PATCH v3 for-next 2/3] net: copy from user before calling
__get_compat_msghdr
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@...hat.com>
> Signed-off-by: Dylan Yudaken <dylany@...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
Powered by blists - more mailing lists