* [PATCH] vhost: handle zero regions in vhost_set_memory
@ 2022-02-21 7:28 Anirudh Rayabharam
2022-02-21 7:56 ` Jason Wang
2022-02-21 16:48 ` Stefano Garzarella
0 siblings, 2 replies; 6+ messages in thread
From: Anirudh Rayabharam @ 2022-02-21 7:28 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang
Cc: mail, syzbot+0abd373e2e50d704db87, kvm, virtualization, netdev,
linux-kernel
Return early when userspace sends zero regions in the VHOST_SET_MEM_TABLE
ioctl.
Otherwise, this causes an erroneous entry to be added to the iotlb. This
entry has a range size of 0 (due to u64 overflow). This then causes
iotlb_access_ok() to loop indefinitely resulting in a hung thread.
Syzbot has reported this here:
https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87
Reported-and-tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com
Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
---
drivers/vhost/vhost.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 59edb5a1ffe2..821aba60eac2 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1428,6 +1428,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
return -EFAULT;
if (mem.padding)
return -EOPNOTSUPP;
+ if (mem.nregions == 0)
+ return 0;
if (mem.nregions > max_mem_regions)
return -E2BIG;
newmem = kvzalloc(struct_size(newmem, regions, mem.nregions),
--
2.35.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: handle zero regions in vhost_set_memory
2022-02-21 7:28 [PATCH] vhost: handle zero regions in vhost_set_memory Anirudh Rayabharam
@ 2022-02-21 7:56 ` Jason Wang
2022-02-21 8:11 ` Anirudh Rayabharam
2022-02-21 16:48 ` Stefano Garzarella
1 sibling, 1 reply; 6+ messages in thread
From: Jason Wang @ 2022-02-21 7:56 UTC (permalink / raw)
To: Anirudh Rayabharam
Cc: Michael S. Tsirkin, syzbot+0abd373e2e50d704db87, kvm,
virtualization, netdev, linux-kernel
On Mon, Feb 21, 2022 at 3:45 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote:
>
> Return early when userspace sends zero regions in the VHOST_SET_MEM_TABLE
> ioctl.
>
> Otherwise, this causes an erroneous entry to be added to the iotlb. This
> entry has a range size of 0 (due to u64 overflow). This then causes
> iotlb_access_ok() to loop indefinitely resulting in a hung thread.
> Syzbot has reported this here:
Interesting, I think iotlb_access_ok() won't be called for memory
table entries, or anything I missed?
(If this is not true, we need a kernel patch as well).
Thanks
>
> https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87
>
> Reported-and-tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com
> Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
> ---
> drivers/vhost/vhost.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 59edb5a1ffe2..821aba60eac2 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -1428,6 +1428,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> return -EFAULT;
> if (mem.padding)
> return -EOPNOTSUPP;
> + if (mem.nregions == 0)
> + return 0;
> if (mem.nregions > max_mem_regions)
> return -E2BIG;
> newmem = kvzalloc(struct_size(newmem, regions, mem.nregions),
> --
> 2.35.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: handle zero regions in vhost_set_memory
2022-02-21 7:56 ` Jason Wang
@ 2022-02-21 8:11 ` Anirudh Rayabharam
0 siblings, 0 replies; 6+ messages in thread
From: Anirudh Rayabharam @ 2022-02-21 8:11 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, syzbot+0abd373e2e50d704db87, kvm,
virtualization, netdev, linux-kernel, mail
On Mon, Feb 21, 2022 at 03:56:29PM +0800, Jason Wang wrote:
> On Mon, Feb 21, 2022 at 3:45 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote:
> >
> > Return early when userspace sends zero regions in the VHOST_SET_MEM_TABLE
> > ioctl.
> >
> > Otherwise, this causes an erroneous entry to be added to the iotlb. This
> > entry has a range size of 0 (due to u64 overflow). This then causes
> > iotlb_access_ok() to loop indefinitely resulting in a hung thread.
> > Syzbot has reported this here:
>
> Interesting, I think iotlb_access_ok() won't be called for memory
> table entries, or anything I missed?
Yeah, vhost_set_memory() doesn't call iotlb_access_ok(). The issue
appears when it is called later in a different code path. Here's the
trace for iotlb_access_ok():
NMI backtrace for cpu 1
CPU: 1 PID: 3633 Comm: vhost-3632 Not tainted 5.17.0-rc3-syzkaller-00029-ge6251ab4551f #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:check_kcov_mode kernel/kcov.c:166 [inline]
RIP: 0010:__sanitizer_cov_trace_pc+0xd/0x60 kernel/kcov.c:200
Code: 00 00 e9 c6 41 66 02 66 0f 1f 44 00 00 48 8b be b0 01 00 00 e8 b4 ff ff ff 31 c0 c3 90 65 8b 05 29 f7 89 7e 89 c1 48 8b 34 24 <81> e1 00 01 00 00 65 48 8b 14 25 00 70 02 00 a9 00 01 ff 00 74 0e
RSP: 0018:ffffc90000cd7c78 EFLAGS: 00000246
RAX: 0000000080000000 RBX: ffff888079ca8a80 RCX: 0000000080000000
RDX: 0000000000000000 RSI: ffffffff86d3f8fb RDI: 0000000000000003
RBP: 0000000000000000 R08: 0000000000000000 R09: ffffc90000cd7c77
R10: ffffffff86d3f8ed R11: 0000000000000001 R12: 0000000000000000
R13: 0000000000000000 R14: 0000000000000001 R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff8880b9d00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007ffdf716a3b8 CR3: 00000000235b6000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
iotlb_access_ok+0x21b/0x3e0 drivers/vhost/vhost.c:1340
vq_meta_prefetch+0xbc/0x280 drivers/vhost/vhost.c:1366
vhost_transport_do_send_pkt+0xe0/0xfd0 drivers/vhost/vsock.c:104
vhost_worker+0x23d/0x3d0 drivers/vhost/vhost.c:372
kthread+0x2e9/0x3a0 kernel/kthread.c:377
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295
</TASK>
----------------
Thanks,
- Anirudh
>
> (If this is not true, we need a kernel patch as well).
>
> Thanks
>
> >
> > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87
> >
> > Reported-and-tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
> > ---
> > drivers/vhost/vhost.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 59edb5a1ffe2..821aba60eac2 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1428,6 +1428,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > return -EFAULT;
> > if (mem.padding)
> > return -EOPNOTSUPP;
> > + if (mem.nregions == 0)
> > + return 0;
> > if (mem.nregions > max_mem_regions)
> > return -E2BIG;
> > newmem = kvzalloc(struct_size(newmem, regions, mem.nregions),
> > --
> > 2.35.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: handle zero regions in vhost_set_memory
2022-02-21 7:28 [PATCH] vhost: handle zero regions in vhost_set_memory Anirudh Rayabharam
2022-02-21 7:56 ` Jason Wang
@ 2022-02-21 16:48 ` Stefano Garzarella
2022-02-21 17:59 ` Anirudh Rayabharam
2022-02-21 19:57 ` Anirudh Rayabharam
1 sibling, 2 replies; 6+ messages in thread
From: Stefano Garzarella @ 2022-02-21 16:48 UTC (permalink / raw)
To: Anirudh Rayabharam
Cc: Michael S. Tsirkin, Jason Wang, syzbot+0abd373e2e50d704db87, kvm,
virtualization, netdev, linux-kernel
On Mon, Feb 21, 2022 at 12:58:51PM +0530, Anirudh Rayabharam wrote:
>Return early when userspace sends zero regions in the VHOST_SET_MEM_TABLE
>ioctl.
>
>Otherwise, this causes an erroneous entry to be added to the iotlb. This
>entry has a range size of 0 (due to u64 overflow). This then causes
>iotlb_access_ok() to loop indefinitely resulting in a hung thread.
>Syzbot has reported this here:
>
>https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87
IIUC vhost_iotlb_add_range() in the for loop is never called if
mem.nregions is 0, so I'm not sure the problem reported by syzbot is
related.
In any case maybe this patch is fine, but currently I think we're just
registering an iotlb without any regions, which in theory shouldn't
cause any problems.
Thanks,
Stefano
>
>Reported-and-tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com
>Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
>---
> drivers/vhost/vhost.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
>index 59edb5a1ffe2..821aba60eac2 100644
>--- a/drivers/vhost/vhost.c
>+++ b/drivers/vhost/vhost.c
>@@ -1428,6 +1428,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> return -EFAULT;
> if (mem.padding)
> return -EOPNOTSUPP;
>+ if (mem.nregions == 0)
>+ return 0;
> if (mem.nregions > max_mem_regions)
> return -E2BIG;
> newmem = kvzalloc(struct_size(newmem, regions, mem.nregions),
>--
>2.35.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: handle zero regions in vhost_set_memory
2022-02-21 16:48 ` Stefano Garzarella
@ 2022-02-21 17:59 ` Anirudh Rayabharam
2022-02-21 19:57 ` Anirudh Rayabharam
1 sibling, 0 replies; 6+ messages in thread
From: Anirudh Rayabharam @ 2022-02-21 17:59 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Michael S. Tsirkin, Jason Wang, syzbot+0abd373e2e50d704db87, kvm,
virtualization, netdev, linux-kernel
On Mon, Feb 21, 2022 at 05:48:17PM +0100, Stefano Garzarella wrote:
> On Mon, Feb 21, 2022 at 12:58:51PM +0530, Anirudh Rayabharam wrote:
> > Return early when userspace sends zero regions in the VHOST_SET_MEM_TABLE
> > ioctl.
> >
> > Otherwise, this causes an erroneous entry to be added to the iotlb. This
> > entry has a range size of 0 (due to u64 overflow). This then causes
> > iotlb_access_ok() to loop indefinitely resulting in a hung thread.
> > Syzbot has reported this here:
> >
> > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87
>
> IIUC vhost_iotlb_add_range() in the for loop is never called if mem.nregions
> is 0, so I'm not sure the problem reported by syzbot is related.
Oh you're right. My bad! The problem is actually elsewhere.
>
> In any case maybe this patch is fine, but currently I think we're just
> registering an iotlb without any regions, which in theory shouldn't cause
> any problems.
Yeah, there shouldn't be any problems here. The problem actually seems
to be in vhost_process_iotlb_msg() where we end up adding a zero sized
region to the iotlb. I will send a new patch...
Thanks!
- Anirudh.
>
> Thanks,
> Stefano
>
> >
> > Reported-and-tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
> > ---
> > drivers/vhost/vhost.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 59edb5a1ffe2..821aba60eac2 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1428,6 +1428,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > return -EFAULT;
> > if (mem.padding)
> > return -EOPNOTSUPP;
> > + if (mem.nregions == 0)
> > + return 0;
> > if (mem.nregions > max_mem_regions)
> > return -E2BIG;
> > newmem = kvzalloc(struct_size(newmem, regions, mem.nregions),
> > --
> > 2.35.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] vhost: handle zero regions in vhost_set_memory
2022-02-21 16:48 ` Stefano Garzarella
2022-02-21 17:59 ` Anirudh Rayabharam
@ 2022-02-21 19:57 ` Anirudh Rayabharam
1 sibling, 0 replies; 6+ messages in thread
From: Anirudh Rayabharam @ 2022-02-21 19:57 UTC (permalink / raw)
To: Stefano Garzarella
Cc: Michael S. Tsirkin, Jason Wang, syzbot+0abd373e2e50d704db87, kvm,
virtualization, netdev, linux-kernel
On Mon, Feb 21, 2022 at 05:48:17PM +0100, Stefano Garzarella wrote:
> On Mon, Feb 21, 2022 at 12:58:51PM +0530, Anirudh Rayabharam wrote:
> > Return early when userspace sends zero regions in the VHOST_SET_MEM_TABLE
> > ioctl.
> >
> > Otherwise, this causes an erroneous entry to be added to the iotlb. This
> > entry has a range size of 0 (due to u64 overflow). This then causes
> > iotlb_access_ok() to loop indefinitely resulting in a hung thread.
> > Syzbot has reported this here:
> >
> > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87
>
> IIUC vhost_iotlb_add_range() in the for loop is never called if mem.nregions
> is 0, so I'm not sure the problem reported by syzbot is related.
>
> In any case maybe this patch is fine, but currently I think we're just
> registering an iotlb without any regions, which in theory shouldn't cause
> any problems.
Sent a new patch: https://lore.kernel.org/lkml/20220221195303.13560-1-mail@anirudhrb.com/T/#u
>
> Thanks,
> Stefano
>
> >
> > Reported-and-tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com
> > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com>
> > ---
> > drivers/vhost/vhost.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > index 59edb5a1ffe2..821aba60eac2 100644
> > --- a/drivers/vhost/vhost.c
> > +++ b/drivers/vhost/vhost.c
> > @@ -1428,6 +1428,8 @@ static long vhost_set_memory(struct vhost_dev *d, struct vhost_memory __user *m)
> > return -EFAULT;
> > if (mem.padding)
> > return -EOPNOTSUPP;
> > + if (mem.nregions == 0)
> > + return 0;
> > if (mem.nregions > max_mem_regions)
> > return -E2BIG;
> > newmem = kvzalloc(struct_size(newmem, regions, mem.nregions),
> > --
> > 2.35.1
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-02-21 19:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-21 7:28 [PATCH] vhost: handle zero regions in vhost_set_memory Anirudh Rayabharam
2022-02-21 7:56 ` Jason Wang
2022-02-21 8:11 ` Anirudh Rayabharam
2022-02-21 16:48 ` Stefano Garzarella
2022-02-21 17:59 ` Anirudh Rayabharam
2022-02-21 19:57 ` Anirudh Rayabharam
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).