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: [email protected]
Signed-off-by: Anirudh Rayabharam <[email protected]>
---
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: [email protected]
> > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > ---
> > 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/[email protected]/T/#u
>
> Thanks,
> Stefano
>
> >
> > Reported-and-tested-by: [email protected]
> > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > ---
> > 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: [email protected]
>Signed-off-by: Anirudh Rayabharam <[email protected]>
>---
> 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
>