| Message ID | 20220221195303.13560-1-mail@anirudhrb.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vhost: validate range size before adding to iotlb | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > before proceeding with adding it to the iotlb. > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > One instance where it can happen is when userspace sends an IOTLB > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > iotlb. Next time a packet is sent, iotlb_access_ok() loops > indefinitely due to that erroneous entry: > > 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> > > Reported by syzbot at: > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > --- > drivers/vhost/iotlb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > index 670d56c879e5..b9de74bd2f9c 100644 > --- a/drivers/vhost/iotlb.c > +++ b/drivers/vhost/iotlb.c > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > void *opaque) > { > struct vhost_iotlb_map *map; > + u64 size = last - start + 1; > > - if (last < start) > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > + if (last < start || size == 0) > return -EFAULT; I'd move this check to vhost_chr_iter_write(), then for the device who has its own msg handler (e.g vDPA) can benefit from it as well. Thanks > > if (iotlb->limit && > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > return -ENOMEM; > > map->start = start; > - map->size = last - start + 1; > + map->size = size; > map->last = last; > map->addr = addr; > map->perm = perm; > -- > 2.35.1 >
On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > before proceeding with adding it to the iotlb. > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > One instance where it can happen is when userspace sends an IOTLB > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > indefinitely due to that erroneous entry: > > > > 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> > > > > Reported by syzbot at: > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > --- > > drivers/vhost/iotlb.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > index 670d56c879e5..b9de74bd2f9c 100644 > > --- a/drivers/vhost/iotlb.c > > +++ b/drivers/vhost/iotlb.c > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > void *opaque) > > { > > struct vhost_iotlb_map *map; > > + u64 size = last - start + 1; > > > > - if (last < start) > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > + if (last < start || size == 0) > > return -EFAULT; > > I'd move this check to vhost_chr_iter_write(), then for the device who > has its own msg handler (e.g vDPA) can benefit from it as well. Thanks for reviewing! I kept the check here thinking that all devices would benefit from it because they would need to call vhost_iotlb_add_range() to add an entry to the iotlb. Isn't that correct? Do you see any other benefit in moving it to vhost_chr_iter_write()? One concern I have is that if we move it out some future caller to vhost_iotlb_add_range() might forget to handle this case. Thanks! - Anirudh. > > Thanks > > > > > if (iotlb->limit && > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > return -ENOMEM; > > > > map->start = start; > > - map->size = last - start + 1; > > + map->size = size; > > map->last = last; > > map->addr = addr; > > map->perm = perm; > > -- > > 2.35.1 > > >
On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > before proceeding with adding it to the iotlb. > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > One instance where it can happen is when userspace sends an IOTLB > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > indefinitely due to that erroneous entry: > > > > > > 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> > > > > > > Reported by syzbot at: > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > --- > > > drivers/vhost/iotlb.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > --- a/drivers/vhost/iotlb.c > > > +++ b/drivers/vhost/iotlb.c > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > void *opaque) > > > { > > > struct vhost_iotlb_map *map; > > > + u64 size = last - start + 1; > > > > > > - if (last < start) > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > + if (last < start || size == 0) > > > return -EFAULT; > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > has its own msg handler (e.g vDPA) can benefit from it as well. > > Thanks for reviewing! > > I kept the check here thinking that all devices would benefit from it > because they would need to call vhost_iotlb_add_range() to add an entry > to the iotlb. Isn't that correct? Correct for now but not for the future, it's not guaranteed that the per device iotlb message handler will use vhost iotlb. But I agree that we probably don't need to care about it too much now. > Do you see any other benefit in moving > it to vhost_chr_iter_write()? > > One concern I have is that if we move it out some future caller to > vhost_iotlb_add_range() might forget to handle this case. Yes. Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range which seems a little bit odd. I wonder if it's better to just remove the map->size. Having a quick glance at the the user, I don't see any blocker for this. Thanks > > Thanks! > > - Anirudh. > > > > > Thanks > > > > > > > > if (iotlb->limit && > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > return -ENOMEM; > > > > > > map->start = start; > > > - map->size = last - start + 1; > > > + map->size = size; > > > map->last = last; > > > map->addr = addr; > > > map->perm = perm; > > > -- > > > 2.35.1 > > > > > >
On Tue, Feb 22, 2022 at 01:23:03AM +0530, Anirudh Rayabharam wrote: > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > before proceeding with adding it to the iotlb. > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > One instance where it can happen is when userspace sends an IOTLB > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > iotlb. Next time a packet is sent, iotlb_access_ok() loops > indefinitely due to that erroneous entry: > > 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> > > Reported by syzbot at: > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > --- > drivers/vhost/iotlb.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > index 670d56c879e5..b9de74bd2f9c 100644 > --- a/drivers/vhost/iotlb.c > +++ b/drivers/vhost/iotlb.c > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > void *opaque) > { > struct vhost_iotlb_map *map; > + u64 size = last - start + 1; > > - if (last < start) > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). Pls use the old-style /* */ comments. > + if (last < start || size == 0) > return -EFAULT; > > if (iotlb->limit && > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > return -ENOMEM; > > map->start = start; > - map->size = last - start + 1; > + map->size = size; > map->last = last; > map->addr = addr; > map->perm = perm; > -- > 2.35.1
On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > before proceeding with adding it to the iotlb. > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > One instance where it can happen is when userspace sends an IOTLB > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > indefinitely due to that erroneous entry: > > > > > > > > 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> > > > > > > > > Reported by syzbot at: > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > --- > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > --- a/drivers/vhost/iotlb.c > > > > +++ b/drivers/vhost/iotlb.c > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > void *opaque) > > > > { > > > > struct vhost_iotlb_map *map; > > > > + u64 size = last - start + 1; > > > > > > > > - if (last < start) > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > + if (last < start || size == 0) > > > > return -EFAULT; > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > Thanks for reviewing! > > > > I kept the check here thinking that all devices would benefit from it > > because they would need to call vhost_iotlb_add_range() to add an entry > > to the iotlb. Isn't that correct? > > Correct for now but not for the future, it's not guaranteed that the > per device iotlb message handler will use vhost iotlb. > > But I agree that we probably don't need to care about it too much now. > > > Do you see any other benefit in moving > > it to vhost_chr_iter_write()? > > > > One concern I have is that if we move it out some future caller to > > vhost_iotlb_add_range() might forget to handle this case. > > Yes. > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > which seems a little bit odd. Well, I guess ideally we'd split this up as two entries - this kind of thing is after all one of the reasons we initially used first,last as the API - as opposed to first,size. Anirudh, could you do it like this instead of rejecting? > I wonder if it's better to just remove > the map->size. Having a quick glance at the the user, I don't see any > blocker for this. > > Thanks I think it's possible but won't solve the bug by itself, and we'd need to review and fix all users - a high chance of introducing another regression. And I think there's value of fitting under the stable rule of 100 lines with context. So sure, but let's fix the bug first. > > > > Thanks! > > > > - Anirudh. > > > > > > > > Thanks > > > > > > > > > > > if (iotlb->limit && > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > return -ENOMEM; > > > > > > > > map->start = start; > > > > - map->size = last - start + 1; > > > > + map->size = size; > > > > map->last = last; > > > > map->addr = addr; > > > > map->perm = perm; > > > > -- > > > > 2.35.1 > > > > > > > > >
On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > 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> > > > > > > > > > > Reported by syzbot at: > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > --- > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > --- a/drivers/vhost/iotlb.c > > > > > +++ b/drivers/vhost/iotlb.c > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > void *opaque) > > > > > { > > > > > struct vhost_iotlb_map *map; > > > > > + u64 size = last - start + 1; > > > > > > > > > > - if (last < start) > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > + if (last < start || size == 0) > > > > > return -EFAULT; > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > Thanks for reviewing! > > > > > > I kept the check here thinking that all devices would benefit from it > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > to the iotlb. Isn't that correct? > > > > Correct for now but not for the future, it's not guaranteed that the > > per device iotlb message handler will use vhost iotlb. > > > > But I agree that we probably don't need to care about it too much now. > > > > > Do you see any other benefit in moving > > > it to vhost_chr_iter_write()? > > > > > > One concern I have is that if we move it out some future caller to > > > vhost_iotlb_add_range() might forget to handle this case. > > > > Yes. > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > which seems a little bit odd. > > Well, I guess ideally we'd split this up as two entries - this kind of > thing is after all one of the reasons we initially used first,last as > the API - as opposed to first,size. IIUC, the APIs exposed to userspace accept first,size. Which means that right now there is now way for userspace to map this range. So, is there any value in not simply rejecting this range? > > Anirudh, could you do it like this instead of rejecting? > > > > I wonder if it's better to just remove > > the map->size. Having a quick glance at the the user, I don't see any > > blocker for this. > > > > Thanks > > I think it's possible but won't solve the bug by itself, and we'd need > to review and fix all users - a high chance of introducing > another regression. Agreed, I did a quick review of the usages and getting rid of size didn't seem trivial. Thanks, - Anirudh. > And I think there's value of fitting under the > stable rule of 100 lines with context. > So sure, but let's fix the bug first. > > > > > > > > > Thanks! > > > > > > - Anirudh. > > > > > > > > > > > Thanks > > > > > > > > > > > > > > if (iotlb->limit && > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > return -ENOMEM; > > > > > > > > > > map->start = start; > > > > > - map->size = last - start + 1; > > > > > + map->size = size; > > > > > map->last = last; > > > > > map->addr = addr; > > > > > map->perm = perm; > > > > > -- > > > > > 2.35.1 > > > > > > > > > > > > >
On Tue, Feb 22, 2022 at 10:57:41PM +0530, Anirudh Rayabharam wrote: > On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote: > > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > > > 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> > > > > > > > > > > > > Reported by syzbot at: > > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > > --- > > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > > --- a/drivers/vhost/iotlb.c > > > > > > +++ b/drivers/vhost/iotlb.c > > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > void *opaque) > > > > > > { > > > > > > struct vhost_iotlb_map *map; > > > > > > + u64 size = last - start + 1; > > > > > > > > > > > > - if (last < start) > > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > + if (last < start || size == 0) > > > > > > return -EFAULT; > > > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > > > Thanks for reviewing! > > > > > > > > I kept the check here thinking that all devices would benefit from it > > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > > to the iotlb. Isn't that correct? > > > > > > Correct for now but not for the future, it's not guaranteed that the > > > per device iotlb message handler will use vhost iotlb. > > > > > > But I agree that we probably don't need to care about it too much now. > > > > > > > Do you see any other benefit in moving > > > > it to vhost_chr_iter_write()? > > > > > > > > One concern I have is that if we move it out some future caller to > > > > vhost_iotlb_add_range() might forget to handle this case. > > > > > > Yes. > > > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > > which seems a little bit odd. > > > > Well, I guess ideally we'd split this up as two entries - this kind of > > thing is after all one of the reasons we initially used first,last as > > the API - as opposed to first,size. > > IIUC, the APIs exposed to userspace accept first,size. Some of them. /* vhost vdpa IOVA range * @first: First address that can be mapped by vhost-vDPA * @last: Last address that can be mapped by vhost-vDPA */ struct vhost_vdpa_iova_range { __u64 first; __u64 last; }; but struct vhost_iotlb_msg { __u64 iova; __u64 size; __u64 uaddr; #define VHOST_ACCESS_RO 0x1 #define VHOST_ACCESS_WO 0x2 #define VHOST_ACCESS_RW 0x3 __u8 perm; #define VHOST_IOTLB_MISS 1 #define VHOST_IOTLB_UPDATE 2 #define VHOST_IOTLB_INVALIDATE 3 #define VHOST_IOTLB_ACCESS_FAIL 4 /* * VHOST_IOTLB_BATCH_BEGIN and VHOST_IOTLB_BATCH_END allow modifying * multiple mappings in one go: beginning with * VHOST_IOTLB_BATCH_BEGIN, followed by any number of * VHOST_IOTLB_UPDATE messages, and ending with VHOST_IOTLB_BATCH_END. * When one of these two values is used as the message type, the rest * of the fields in the message are ignored. There's no guarantee that * these changes take place automatically in the device. */ #define VHOST_IOTLB_BATCH_BEGIN 5 #define VHOST_IOTLB_BATCH_END 6 __u8 type; }; > Which means that > right now there is now way for userspace to map this range. So, is there > any value in not simply rejecting this range? > > > > > Anirudh, could you do it like this instead of rejecting? > > > > > > > I wonder if it's better to just remove > > > the map->size. Having a quick glance at the the user, I don't see any > > > blocker for this. > > > > > > Thanks > > > > I think it's possible but won't solve the bug by itself, and we'd need > > to review and fix all users - a high chance of introducing > > another regression. > > Agreed, I did a quick review of the usages and getting rid of size > didn't seem trivial. > > Thanks, > > - Anirudh. > > > And I think there's value of fitting under the > > stable rule of 100 lines with context. > > So sure, but let's fix the bug first. > > > > > > > > > > > > > > Thanks! > > > > > > > > - Anirudh. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > if (iotlb->limit && > > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > return -ENOMEM; > > > > > > > > > > > > map->start = start; > > > > > > - map->size = last - start + 1; > > > > > > + map->size = size; > > > > > > map->last = last; > > > > > > map->addr = addr; > > > > > > map->perm = perm; > > > > > > -- > > > > > > 2.35.1 > > > > > > > > > > > > > > > > >
On Tue, Feb 22, 2022 at 11:02 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > 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> > > > > > > > > > > Reported by syzbot at: > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > --- > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > --- a/drivers/vhost/iotlb.c > > > > > +++ b/drivers/vhost/iotlb.c > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > void *opaque) > > > > > { > > > > > struct vhost_iotlb_map *map; > > > > > + u64 size = last - start + 1; > > > > > > > > > > - if (last < start) > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > + if (last < start || size == 0) > > > > > return -EFAULT; > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > Thanks for reviewing! > > > > > > I kept the check here thinking that all devices would benefit from it > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > to the iotlb. Isn't that correct? > > > > Correct for now but not for the future, it's not guaranteed that the > > per device iotlb message handler will use vhost iotlb. > > > > But I agree that we probably don't need to care about it too much now. > > > > > Do you see any other benefit in moving > > > it to vhost_chr_iter_write()? > > > > > > One concern I have is that if we move it out some future caller to > > > vhost_iotlb_add_range() might forget to handle this case. > > > > Yes. > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > which seems a little bit odd. > > Well, I guess ideally we'd split this up as two entries - this kind of > thing is after all one of the reasons we initially used first,last as > the API - as opposed to first,size. > > Anirudh, could you do it like this instead of rejecting? > > > > I wonder if it's better to just remove > > the map->size. Having a quick glance at the the user, I don't see any > > blocker for this. > > > > Thanks > > I think it's possible but won't solve the bug by itself, and we'd need > to review and fix all users - a high chance of introducing > another regression. And I think there's value of fitting under the > stable rule of 100 lines with context. > So sure, but let's fix the bug first. Ok, I agree. Thanks > > > > > > > > > Thanks! > > > > > > - Anirudh. > > > > > > > > > > > Thanks > > > > > > > > > > > > > > if (iotlb->limit && > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > return -ENOMEM; > > > > > > > > > > map->start = start; > > > > > - map->size = last - start + 1; > > > > > + map->size = size; > > > > > map->last = last; > > > > > map->addr = addr; > > > > > map->perm = perm; > > > > > -- > > > > > 2.35.1 > > > > > > > > > > > > >
On Tue, Feb 22, 2022 at 06:21:50PM -0500, Michael S. Tsirkin wrote: > On Tue, Feb 22, 2022 at 10:57:41PM +0530, Anirudh Rayabharam wrote: > > On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote: > > > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > > > > > 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> > > > > > > > > > > > > > > Reported by syzbot at: > > > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > > > --- > > > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > > > --- a/drivers/vhost/iotlb.c > > > > > > > +++ b/drivers/vhost/iotlb.c > > > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > void *opaque) > > > > > > > { > > > > > > > struct vhost_iotlb_map *map; > > > > > > > + u64 size = last - start + 1; > > > > > > > > > > > > > > - if (last < start) > > > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > + if (last < start || size == 0) > > > > > > > return -EFAULT; > > > > > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > > > > > Thanks for reviewing! > > > > > > > > > > I kept the check here thinking that all devices would benefit from it > > > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > > > to the iotlb. Isn't that correct? > > > > > > > > Correct for now but not for the future, it's not guaranteed that the > > > > per device iotlb message handler will use vhost iotlb. > > > > > > > > But I agree that we probably don't need to care about it too much now. > > > > > > > > > Do you see any other benefit in moving > > > > > it to vhost_chr_iter_write()? > > > > > > > > > > One concern I have is that if we move it out some future caller to > > > > > vhost_iotlb_add_range() might forget to handle this case. > > > > > > > > Yes. > > > > > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > > > which seems a little bit odd. > > > > > > Well, I guess ideally we'd split this up as two entries - this kind of > > > thing is after all one of the reasons we initially used first,last as > > > the API - as opposed to first,size. > > > > IIUC, the APIs exposed to userspace accept first,size. > > Some of them. > > > /* vhost vdpa IOVA range > * @first: First address that can be mapped by vhost-vDPA > * @last: Last address that can be mapped by vhost-vDPA > */ > struct vhost_vdpa_iova_range { > __u64 first; > __u64 last; > }; Alright, I will split it into two entries. That doesn't fully address the bug though. I would also need to validate size in vhost_chr_iter_write(). Should I do both in one patch or as a two patch series? > > but > > struct vhost_iotlb_msg { > __u64 iova; > __u64 size; > __u64 uaddr; > #define VHOST_ACCESS_RO 0x1 > #define VHOST_ACCESS_WO 0x2 > #define VHOST_ACCESS_RW 0x3 > __u8 perm; > #define VHOST_IOTLB_MISS 1 > #define VHOST_IOTLB_UPDATE 2 > #define VHOST_IOTLB_INVALIDATE 3 > #define VHOST_IOTLB_ACCESS_FAIL 4 > /* > * VHOST_IOTLB_BATCH_BEGIN and VHOST_IOTLB_BATCH_END allow modifying > * multiple mappings in one go: beginning with > * VHOST_IOTLB_BATCH_BEGIN, followed by any number of > * VHOST_IOTLB_UPDATE messages, and ending with VHOST_IOTLB_BATCH_END. > * When one of these two values is used as the message type, the rest > * of the fields in the message are ignored. There's no guarantee that > * these changes take place automatically in the device. > */ > #define VHOST_IOTLB_BATCH_BEGIN 5 > #define VHOST_IOTLB_BATCH_END 6 > __u8 type; > }; > > > > > Which means that > > right now there is now way for userspace to map this range. So, is there > > any value in not simply rejecting this range? > > > > > > > > Anirudh, could you do it like this instead of rejecting? > > > > > > > > > > I wonder if it's better to just remove > > > > the map->size. Having a quick glance at the the user, I don't see any > > > > blocker for this. > > > > > > > > Thanks > > > > > > I think it's possible but won't solve the bug by itself, and we'd need > > > to review and fix all users - a high chance of introducing > > > another regression. > > > > Agreed, I did a quick review of the usages and getting rid of size > > didn't seem trivial. > > > > Thanks, > > > > - Anirudh. > > > > > And I think there's value of fitting under the > > > stable rule of 100 lines with context. > > > So sure, but let's fix the bug first. > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > - Anirudh. > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > if (iotlb->limit && > > > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > return -ENOMEM; > > > > > > > > > > > > > > map->start = start; > > > > > > > - map->size = last - start + 1; > > > > > > > + map->size = size; > > > > > > > map->last = last; > > > > > > > map->addr = addr; > > > > > > > map->perm = perm; > > > > > > > -- > > > > > > > 2.35.1 > > > > > > > > > > > > > > > > > > > > > >
On Wed, Feb 23, 2022 at 07:48:18PM +0530, Anirudh Rayabharam wrote: > On Tue, Feb 22, 2022 at 06:21:50PM -0500, Michael S. Tsirkin wrote: > > On Tue, Feb 22, 2022 at 10:57:41PM +0530, Anirudh Rayabharam wrote: > > > On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote: > > > > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > > > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > > > > > > > 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> > > > > > > > > > > > > > > > > Reported by syzbot at: > > > > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > > > > --- > > > > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > > > > --- a/drivers/vhost/iotlb.c > > > > > > > > +++ b/drivers/vhost/iotlb.c > > > > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > > void *opaque) > > > > > > > > { > > > > > > > > struct vhost_iotlb_map *map; > > > > > > > > + u64 size = last - start + 1; > > > > > > > > > > > > > > > > - if (last < start) > > > > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > > + if (last < start || size == 0) > > > > > > > > return -EFAULT; > > > > > > > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > > > > > > > Thanks for reviewing! > > > > > > > > > > > > I kept the check here thinking that all devices would benefit from it > > > > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > > > > to the iotlb. Isn't that correct? > > > > > > > > > > Correct for now but not for the future, it's not guaranteed that the > > > > > per device iotlb message handler will use vhost iotlb. > > > > > > > > > > But I agree that we probably don't need to care about it too much now. > > > > > > > > > > > Do you see any other benefit in moving > > > > > > it to vhost_chr_iter_write()? > > > > > > > > > > > > One concern I have is that if we move it out some future caller to > > > > > > vhost_iotlb_add_range() might forget to handle this case. > > > > > > > > > > Yes. > > > > > > > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > > > > which seems a little bit odd. > > > > > > > > Well, I guess ideally we'd split this up as two entries - this kind of > > > > thing is after all one of the reasons we initially used first,last as > > > > the API - as opposed to first,size. > > > > > > IIUC, the APIs exposed to userspace accept first,size. > > > > Some of them. > > > > > > /* vhost vdpa IOVA range > > * @first: First address that can be mapped by vhost-vDPA > > * @last: Last address that can be mapped by vhost-vDPA > > */ > > struct vhost_vdpa_iova_range { > > __u64 first; > > __u64 last; > > }; > > Alright, I will split it into two entries. That doesn't fully address > the bug though. I would also need to validate size in vhost_chr_iter_write(). Do you mean vhost_chr_write_iter? > > Should I do both in one patch or as a two patch series? I'm not sure why we need to do validation in vhost_chr_iter_write, hard to say without seeing the patch. > > > > but > > > > struct vhost_iotlb_msg { > > __u64 iova; > > __u64 size; > > __u64 uaddr; > > #define VHOST_ACCESS_RO 0x1 > > #define VHOST_ACCESS_WO 0x2 > > #define VHOST_ACCESS_RW 0x3 > > __u8 perm; > > #define VHOST_IOTLB_MISS 1 > > #define VHOST_IOTLB_UPDATE 2 > > #define VHOST_IOTLB_INVALIDATE 3 > > #define VHOST_IOTLB_ACCESS_FAIL 4 > > /* > > * VHOST_IOTLB_BATCH_BEGIN and VHOST_IOTLB_BATCH_END allow modifying > > * multiple mappings in one go: beginning with > > * VHOST_IOTLB_BATCH_BEGIN, followed by any number of > > * VHOST_IOTLB_UPDATE messages, and ending with VHOST_IOTLB_BATCH_END. > > * When one of these two values is used as the message type, the rest > > * of the fields in the message are ignored. There's no guarantee that > > * these changes take place automatically in the device. > > */ > > #define VHOST_IOTLB_BATCH_BEGIN 5 > > #define VHOST_IOTLB_BATCH_END 6 > > __u8 type; > > }; > > > > > > > > > Which means that > > > right now there is now way for userspace to map this range. So, is there > > > any value in not simply rejecting this range? > > > > > > > > > > > Anirudh, could you do it like this instead of rejecting? > > > > > > > > > > > > > I wonder if it's better to just remove > > > > > the map->size. Having a quick glance at the the user, I don't see any > > > > > blocker for this. > > > > > > > > > > Thanks > > > > > > > > I think it's possible but won't solve the bug by itself, and we'd need > > > > to review and fix all users - a high chance of introducing > > > > another regression. > > > > > > Agreed, I did a quick review of the usages and getting rid of size > > > didn't seem trivial. > > > > > > Thanks, > > > > > > - Anirudh. > > > > > > > And I think there's value of fitting under the > > > > stable rule of 100 lines with context. > > > > So sure, but let's fix the bug first. > > > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > - Anirudh. > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > if (iotlb->limit && > > > > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > > return -ENOMEM; > > > > > > > > > > > > > > > > map->start = start; > > > > > > > > - map->size = last - start + 1; > > > > > > > > + map->size = size; > > > > > > > > map->last = last; > > > > > > > > map->addr = addr; > > > > > > > > map->perm = perm; > > > > > > > > -- > > > > > > > > 2.35.1 > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Wed, Feb 23, 2022 at 10:15:01AM -0500, Michael S. Tsirkin wrote: > On Wed, Feb 23, 2022 at 07:48:18PM +0530, Anirudh Rayabharam wrote: > > On Tue, Feb 22, 2022 at 06:21:50PM -0500, Michael S. Tsirkin wrote: > > > On Tue, Feb 22, 2022 at 10:57:41PM +0530, Anirudh Rayabharam wrote: > > > > On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote: > > > > > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > > > > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > > > > > > > > > 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> > > > > > > > > > > > > > > > > > > Reported by syzbot at: > > > > > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > > > > > --- > > > > > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > > > > > --- a/drivers/vhost/iotlb.c > > > > > > > > > +++ b/drivers/vhost/iotlb.c > > > > > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > > > void *opaque) > > > > > > > > > { > > > > > > > > > struct vhost_iotlb_map *map; > > > > > > > > > + u64 size = last - start + 1; > > > > > > > > > > > > > > > > > > - if (last < start) > > > > > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > > > + if (last < start || size == 0) > > > > > > > > > return -EFAULT; > > > > > > > > > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > > > > > > > > > Thanks for reviewing! > > > > > > > > > > > > > > I kept the check here thinking that all devices would benefit from it > > > > > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > > > > > to the iotlb. Isn't that correct? > > > > > > > > > > > > Correct for now but not for the future, it's not guaranteed that the > > > > > > per device iotlb message handler will use vhost iotlb. > > > > > > > > > > > > But I agree that we probably don't need to care about it too much now. > > > > > > > > > > > > > Do you see any other benefit in moving > > > > > > > it to vhost_chr_iter_write()? > > > > > > > > > > > > > > One concern I have is that if we move it out some future caller to > > > > > > > vhost_iotlb_add_range() might forget to handle this case. > > > > > > > > > > > > Yes. > > > > > > > > > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > > > > > which seems a little bit odd. > > > > > > > > > > Well, I guess ideally we'd split this up as two entries - this kind of > > > > > thing is after all one of the reasons we initially used first,last as > > > > > the API - as opposed to first,size. > > > > > > > > IIUC, the APIs exposed to userspace accept first,size. > > > > > > Some of them. > > > > > > > > > /* vhost vdpa IOVA range > > > * @first: First address that can be mapped by vhost-vDPA > > > * @last: Last address that can be mapped by vhost-vDPA > > > */ > > > struct vhost_vdpa_iova_range { > > > __u64 first; > > > __u64 last; > > > }; > > > > Alright, I will split it into two entries. That doesn't fully address > > the bug though. I would also need to validate size in vhost_chr_iter_write(). > > Do you mean vhost_chr_write_iter? Yes, my bad. > > > > > Should I do both in one patch or as a two patch series? > > I'm not sure why we need to do validation in vhost_chr_iter_write, > hard to say without seeing the patch. Well, if userspace sends iova = 0 and size = 0 in vhost_iotlb_msg, we will end up mapping the range [0, ULONG_MAX] in iotlb which doesn't make sense. We should probably reject when size = 0. As Jason pointed out [1], having the check in vhost_chr_write_iter() will also benefit devices that have their own message handler. [1]: https://lore.kernel.org/kvm/CACGkMEvLE=kV4PxJLRjdSyKArU+MRx6b_mbLGZHSUgoAAZ+-Fg@mail.gmail.com/ > > > > > > > but > > > > > > struct vhost_iotlb_msg { > > > __u64 iova; > > > __u64 size; > > > __u64 uaddr; > > > #define VHOST_ACCESS_RO 0x1 > > > #define VHOST_ACCESS_WO 0x2 > > > #define VHOST_ACCESS_RW 0x3 > > > __u8 perm; > > > #define VHOST_IOTLB_MISS 1 > > > #define VHOST_IOTLB_UPDATE 2 > > > #define VHOST_IOTLB_INVALIDATE 3 > > > #define VHOST_IOTLB_ACCESS_FAIL 4 > > > /* > > > * VHOST_IOTLB_BATCH_BEGIN and VHOST_IOTLB_BATCH_END allow modifying > > > * multiple mappings in one go: beginning with > > > * VHOST_IOTLB_BATCH_BEGIN, followed by any number of > > > * VHOST_IOTLB_UPDATE messages, and ending with VHOST_IOTLB_BATCH_END. > > > * When one of these two values is used as the message type, the rest > > > * of the fields in the message are ignored. There's no guarantee that > > > * these changes take place automatically in the device. > > > */ > > > #define VHOST_IOTLB_BATCH_BEGIN 5 > > > #define VHOST_IOTLB_BATCH_END 6 > > > __u8 type; > > > }; > > > > > > > > > > > > > Which means that > > > > right now there is now way for userspace to map this range. So, is there > > > > any value in not simply rejecting this range? > > > > > > > > > > > > > > Anirudh, could you do it like this instead of rejecting? > > > > > > > > > > > > > > > > I wonder if it's better to just remove > > > > > > the map->size. Having a quick glance at the the user, I don't see any > > > > > > blocker for this. > > > > > > > > > > > > Thanks > > > > > > > > > > I think it's possible but won't solve the bug by itself, and we'd need > > > > > to review and fix all users - a high chance of introducing > > > > > another regression. > > > > > > > > Agreed, I did a quick review of the usages and getting rid of size > > > > didn't seem trivial. > > > > > > > > Thanks, > > > > > > > > - Anirudh. > > > > > > > > > And I think there's value of fitting under the > > > > > stable rule of 100 lines with context. > > > > > So sure, but let's fix the bug first. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > - Anirudh. > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > if (iotlb->limit && > > > > > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > > > return -ENOMEM; > > > > > > > > > > > > > > > > > > map->start = start; > > > > > > > > > - map->size = last - start + 1; > > > > > > > > > + map->size = size; > > > > > > > > > map->last = last; > > > > > > > > > map->addr = addr; > > > > > > > > > map->perm = perm; > > > > > > > > > -- > > > > > > > > > 2.35.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Wed, Feb 23, 2022 at 10:19:23PM +0530, Anirudh Rayabharam wrote: > On Wed, Feb 23, 2022 at 10:15:01AM -0500, Michael S. Tsirkin wrote: > > On Wed, Feb 23, 2022 at 07:48:18PM +0530, Anirudh Rayabharam wrote: > > > On Tue, Feb 22, 2022 at 06:21:50PM -0500, Michael S. Tsirkin wrote: > > > > On Tue, Feb 22, 2022 at 10:57:41PM +0530, Anirudh Rayabharam wrote: > > > > > On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote: > > > > > > On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote: > > > > > > > On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > > > > > On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote: > > > > > > > > > On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <mail@anirudhrb.com> wrote: > > > > > > > > > > > > > > > > > > > > In vhost_iotlb_add_range_ctx(), validate the range size is non-zero > > > > > > > > > > before proceeding with adding it to the iotlb. > > > > > > > > > > > > > > > > > > > > Range size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > > > > One instance where it can happen is when userspace sends an IOTLB > > > > > > > > > > message with iova=size=uaddr=0 (vhost_process_iotlb_msg). So, an > > > > > > > > > > entry with size = 0, start = 0, last = (2^64 - 1) ends up in the > > > > > > > > > > iotlb. Next time a packet is sent, iotlb_access_ok() loops > > > > > > > > > > indefinitely due to that erroneous entry: > > > > > > > > > > > > > > > > > > > > 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> > > > > > > > > > > > > > > > > > > > > Reported by syzbot at: > > > > > > > > > > https://syzkaller.appspot.com/bug?extid=0abd373e2e50d704db87 > > > > > > > > > > > > > > > > > > > > Reported-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > > > > Tested-by: syzbot+0abd373e2e50d704db87@syzkaller.appspotmail.com > > > > > > > > > > Signed-off-by: Anirudh Rayabharam <mail@anirudhrb.com> > > > > > > > > > > --- > > > > > > > > > > drivers/vhost/iotlb.c | 6 ++++-- > > > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c > > > > > > > > > > index 670d56c879e5..b9de74bd2f9c 100644 > > > > > > > > > > --- a/drivers/vhost/iotlb.c > > > > > > > > > > +++ b/drivers/vhost/iotlb.c > > > > > > > > > > @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > > > > void *opaque) > > > > > > > > > > { > > > > > > > > > > struct vhost_iotlb_map *map; > > > > > > > > > > + u64 size = last - start + 1; > > > > > > > > > > > > > > > > > > > > - if (last < start) > > > > > > > > > > + // size can overflow to 0 when start is 0 and last is (2^64 - 1). > > > > > > > > > > + if (last < start || size == 0) > > > > > > > > > > return -EFAULT; > > > > > > > > > > > > > > > > > > I'd move this check to vhost_chr_iter_write(), then for the device who > > > > > > > > > has its own msg handler (e.g vDPA) can benefit from it as well. > > > > > > > > > > > > > > > > Thanks for reviewing! > > > > > > > > > > > > > > > > I kept the check here thinking that all devices would benefit from it > > > > > > > > because they would need to call vhost_iotlb_add_range() to add an entry > > > > > > > > to the iotlb. Isn't that correct? > > > > > > > > > > > > > > Correct for now but not for the future, it's not guaranteed that the > > > > > > > per device iotlb message handler will use vhost iotlb. > > > > > > > > > > > > > > But I agree that we probably don't need to care about it too much now. > > > > > > > > > > > > > > > Do you see any other benefit in moving > > > > > > > > it to vhost_chr_iter_write()? > > > > > > > > > > > > > > > > One concern I have is that if we move it out some future caller to > > > > > > > > vhost_iotlb_add_range() might forget to handle this case. > > > > > > > > > > > > > > Yes. > > > > > > > > > > > > > > Rethink the whole fix, we're basically rejecting [0, ULONG_MAX] range > > > > > > > which seems a little bit odd. > > > > > > > > > > > > Well, I guess ideally we'd split this up as two entries - this kind of > > > > > > thing is after all one of the reasons we initially used first,last as > > > > > > the API - as opposed to first,size. > > > > > > > > > > IIUC, the APIs exposed to userspace accept first,size. > > > > > > > > Some of them. > > > > > > > > > > > > /* vhost vdpa IOVA range > > > > * @first: First address that can be mapped by vhost-vDPA > > > > * @last: Last address that can be mapped by vhost-vDPA > > > > */ > > > > struct vhost_vdpa_iova_range { > > > > __u64 first; > > > > __u64 last; > > > > }; > > > > > > Alright, I will split it into two entries. That doesn't fully address > > > the bug though. I would also need to validate size in vhost_chr_iter_write(). > > > > Do you mean vhost_chr_write_iter? > > Yes, my bad. > > > > > > > > > Should I do both in one patch or as a two patch series? > > > > I'm not sure why we need to do validation in vhost_chr_iter_write, > > hard to say without seeing the patch. > > Well, if userspace sends iova = 0 and size = 0 in vhost_iotlb_msg, we will end > up mapping the range [0, ULONG_MAX] in iotlb which doesn't make sense. We > should probably reject when size = 0. > > As Jason pointed out [1], having the check in vhost_chr_write_iter() will > also benefit devices that have their own message handler. > > [1]: https://lore.kernel.org/kvm/CACGkMEvLE=kV4PxJLRjdSyKArU+MRx6b_mbLGZHSUgoAAZ+-Fg@mail.gmail.com/ Oh. Makes sense. I think one patch is enough. > > > > > > > > > > but > > > > > > > > struct vhost_iotlb_msg { > > > > __u64 iova; > > > > __u64 size; > > > > __u64 uaddr; > > > > #define VHOST_ACCESS_RO 0x1 > > > > #define VHOST_ACCESS_WO 0x2 > > > > #define VHOST_ACCESS_RW 0x3 > > > > __u8 perm; > > > > #define VHOST_IOTLB_MISS 1 > > > > #define VHOST_IOTLB_UPDATE 2 > > > > #define VHOST_IOTLB_INVALIDATE 3 > > > > #define VHOST_IOTLB_ACCESS_FAIL 4 > > > > /* > > > > * VHOST_IOTLB_BATCH_BEGIN and VHOST_IOTLB_BATCH_END allow modifying > > > > * multiple mappings in one go: beginning with > > > > * VHOST_IOTLB_BATCH_BEGIN, followed by any number of > > > > * VHOST_IOTLB_UPDATE messages, and ending with VHOST_IOTLB_BATCH_END. > > > > * When one of these two values is used as the message type, the rest > > > > * of the fields in the message are ignored. There's no guarantee that > > > > * these changes take place automatically in the device. > > > > */ > > > > #define VHOST_IOTLB_BATCH_BEGIN 5 > > > > #define VHOST_IOTLB_BATCH_END 6 > > > > __u8 type; > > > > }; > > > > > > > > > > > > > > > > > Which means that > > > > > right now there is now way for userspace to map this range. So, is there > > > > > any value in not simply rejecting this range? > > > > > > > > > > > > > > > > > Anirudh, could you do it like this instead of rejecting? > > > > > > > > > > > > > > > > > > > I wonder if it's better to just remove > > > > > > > the map->size. Having a quick glance at the the user, I don't see any > > > > > > > blocker for this. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > I think it's possible but won't solve the bug by itself, and we'd need > > > > > > to review and fix all users - a high chance of introducing > > > > > > another regression. > > > > > > > > > > Agreed, I did a quick review of the usages and getting rid of size > > > > > didn't seem trivial. > > > > > > > > > > Thanks, > > > > > > > > > > - Anirudh. > > > > > > > > > > > And I think there's value of fitting under the > > > > > > stable rule of 100 lines with context. > > > > > > So sure, but let's fix the bug first. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks! > > > > > > > > > > > > > > > > - Anirudh. > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > if (iotlb->limit && > > > > > > > > > > @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, > > > > > > > > > > return -ENOMEM; > > > > > > > > > > > > > > > > > > > > map->start = start; > > > > > > > > > > - map->size = last - start + 1; > > > > > > > > > > + map->size = size; > > > > > > > > > > map->last = last; > > > > > > > > > > map->addr = addr; > > > > > > > > > > map->perm = perm; > > > > > > > > > > -- > > > > > > > > > > 2.35.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/vhost/iotlb.c b/drivers/vhost/iotlb.c index 670d56c879e5..b9de74bd2f9c 100644 --- a/drivers/vhost/iotlb.c +++ b/drivers/vhost/iotlb.c @@ -53,8 +53,10 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, void *opaque) { struct vhost_iotlb_map *map; + u64 size = last - start + 1; - if (last < start) + // size can overflow to 0 when start is 0 and last is (2^64 - 1). + if (last < start || size == 0) return -EFAULT; if (iotlb->limit && @@ -69,7 +71,7 @@ int vhost_iotlb_add_range_ctx(struct vhost_iotlb *iotlb, return -ENOMEM; map->start = start; - map->size = last - start + 1; + map->size = size; map->last = last; map->addr = addr; map->perm = perm;