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:

quoted

On Tue, Feb 22, 2022 at 10:57:41PM +0530, Anirudh Rayabharam wrote:

quoted

On Tue, Feb 22, 2022 at 10:02:29AM -0500, Michael S. Tsirkin wrote:

quoted

On Tue, Feb 22, 2022 at 03:11:07PM +0800, Jason Wang wrote:

quoted

On Tue, Feb 22, 2022 at 12:57 PM Anirudh Rayabharam [off-list ref] wrote:

quoted

On Tue, Feb 22, 2022 at 10:50:20AM +0800, Jason Wang wrote:

quoted

On Tue, Feb 22, 2022 at 3:53 AM Anirudh Rayabharam [off-list ref] wrote:

quoted

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 <redacted>
---
 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.

quoted

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.

quoted

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;
};

quoted

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?

quoted

Anirudh, could you do it like this instead of rejecting?

quoted

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.

quoted

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.

quoted

quoted

Thanks!

        - Anirudh.

quoted

Thanks

quoted

        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