2022-02-22 05:13:04

by Jason Wang

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <[email protected]> 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: [email protected]
> Tested-by: [email protected]
> Signed-off-by: Anirudh Rayabharam <[email protected]>
> ---
> 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
>


2022-02-22 06:07:48

by Anirudh Rayabharam

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote:
> On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam <[email protected]> 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: [email protected]
> > Tested-by: [email protected]
> > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > ---
> > 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
> >
>

2022-02-22 07:11:29

by Jason Wang

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <[email protected]> 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 <[email protected]> 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: [email protected]
> > > Tested-by: [email protected]
> > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > ---
> > > 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
> > >
> >
>

2022-02-22 15:11:31

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote:
> On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > Tested-by: [email protected]
> > > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > > ---
> > > > 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
> > > >
> > >
> >

2022-02-23 02:29:47

by Anirudh Rayabharam

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > Tested-by: [email protected]
> > > > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > > > ---
> > > > > 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
> > > > >
> > > >
> > >
>

2022-02-23 02:36:09

by Jason Wang

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

On Tue, Feb 22, 2022 at 11:02 PM Michael S. Tsirkin <[email protected]> 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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > Tested-by: [email protected]
> > > > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > > > ---
> > > > > 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
> > > > >
> > > >
> > >
>

2022-02-23 02:36:12

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > > Tested-by: [email protected]
> > > > > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > > > > ---
> > > > > > 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
> > > > > >
> > > > >
> > > >
> >

2022-02-23 17:45:19

by Anirudh Rayabharam

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > > > Tested-by: [email protected]
> > > > > > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > > > > > ---
> > > > > > > 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
> > > > > > >
> > > > > >
> > > > >
> > >
>

2022-02-24 00:36:44

by Michael S. Tsirkin

[permalink] [raw]
Subject: Re: [PATCH] vhost: validate range size before adding to iotlb

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 <[email protected]> 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 <[email protected]> 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: [email protected]
> > > > > > > > Tested-by: [email protected]
> > > > > > > > Signed-off-by: Anirudh Rayabharam <[email protected]>
> > > > > > > > ---
> > > > > > > > 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
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> >