| Message ID | 20220221072852.31820-1-mail@anirudhrb.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vhost: handle zero regions in vhost_set_memory | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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 >
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 > > >
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 >
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 > > >
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 > > >
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),
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(+)