hw/virtio/virtio.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
In virtqueue_{split,packed}_get_avail_bytes() descriptors are read
in a loop via MemoryRegionCache regions and calls to
vring_{split,packed}_desc_read() - these take a region cache and the
index of the descriptor to be read.
For direct descriptors we use a cache provided by the caller, whose
size matches that of the virtqueue vring. We limit the number of
descriptors we can read by the size of that vring:
max = vq->vring.num;
...
MemoryRegionCache *desc_cache = &caches->desc;
For indirect descriptors, we initialize a new cache and limit the
number of descriptors by the size of the intermediate descriptor:
len = address_space_cache_init(&indirect_desc_cache,
vdev->dma_as,
desc.addr, desc.len, false);
desc_cache = &indirect_desc_cache;
...
max = desc.len / sizeof(VRingDesc);
However, the first initialization of `max` is done outside the loop
where we process guest descriptors, while the second one is done
inside. This means that a sequence of an indirect descriptor followed
by a direct one will leave a stale value in `max`. If the second
descriptor's `next` field is smaller than the stale value, but
greater than the size of the virtqueue ring (and thus the cached
region), a failed assertion will be triggered in
address_space_read_cached() down the call chain.
Fix this by initializing `max` inside the loop in both functions.
Fixes: 9796d0ac8fb0 ("virtio: use address_space_map/unmap to access descriptors")
Signed-off-by: Carlos López <clopez@suse.de>
---
hw/virtio/virtio.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f35178f5fc..db70c4976e 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1071,6 +1071,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
VirtIODevice *vdev = vq->vdev;
unsigned int max, idx;
unsigned int total_bufs, in_total, out_total;
+ MemoryRegionCache *desc_cache;
MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID;
int64_t len = 0;
int rc;
@@ -1078,15 +1079,13 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq,
idx = vq->last_avail_idx;
total_bufs = in_total = out_total = 0;
- max = vq->vring.num;
-
while ((rc = virtqueue_num_heads(vq, idx)) > 0) {
- MemoryRegionCache *desc_cache = &caches->desc;
- unsigned int num_bufs;
+ unsigned int num_bufs = total_bufs;
VRingDesc desc;
unsigned int i;
- num_bufs = total_bufs;
+ desc_cache = &caches->desc;
+ max = vq->vring.num;
if (!virtqueue_get_head(vq, idx++, &i)) {
goto err;
@@ -1218,14 +1217,14 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue *vq,
wrap_counter = vq->last_avail_wrap_counter;
total_bufs = in_total = out_total = 0;
- max = vq->vring.num;
-
for (;;) {
unsigned int num_bufs = total_bufs;
unsigned int i = idx;
int rc;
desc_cache = &caches->desc;
+ max = vq->vring.num;
+
vring_packed_desc_read(vdev, &desc, desc_cache, idx, true);
if (!is_desc_avail(desc.flags, wrap_counter)) {
break;
--
2.35.3
On Thu, Feb 16, 2023 at 6:23 AM Carlos López <clopez@suse.de> wrote: > > In virtqueue_{split,packed}_get_avail_bytes() descriptors are read > in a loop via MemoryRegionCache regions and calls to > vring_{split,packed}_desc_read() - these take a region cache and the > index of the descriptor to be read. > > For direct descriptors we use a cache provided by the caller, whose > size matches that of the virtqueue vring. We limit the number of > descriptors we can read by the size of that vring: > > max = vq->vring.num; > ... > MemoryRegionCache *desc_cache = &caches->desc; > > For indirect descriptors, we initialize a new cache and limit the > number of descriptors by the size of the intermediate descriptor: > > len = address_space_cache_init(&indirect_desc_cache, > vdev->dma_as, > desc.addr, desc.len, false); So desc.addr and desc.len are under the control of the driver. A malicious driver can choose to do a trick there. Should we sanitize them here? Thanks > desc_cache = &indirect_desc_cache; > ... > max = desc.len / sizeof(VRingDesc); > > However, the first initialization of `max` is done outside the loop > where we process guest descriptors, while the second one is done > inside. This means that a sequence of an indirect descriptor followed > by a direct one will leave a stale value in `max`. If the second > descriptor's `next` field is smaller than the stale value, but > greater than the size of the virtqueue ring (and thus the cached > region), a failed assertion will be triggered in > address_space_read_cached() down the call chain. > > Fix this by initializing `max` inside the loop in both functions. > > Fixes: 9796d0ac8fb0 ("virtio: use address_space_map/unmap to access descriptors") > Signed-off-by: Carlos López <clopez@suse.de> > --- > hw/virtio/virtio.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index f35178f5fc..db70c4976e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1071,6 +1071,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, > VirtIODevice *vdev = vq->vdev; > unsigned int max, idx; > unsigned int total_bufs, in_total, out_total; > + MemoryRegionCache *desc_cache; > MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; > int64_t len = 0; > int rc; > @@ -1078,15 +1079,13 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, > idx = vq->last_avail_idx; > total_bufs = in_total = out_total = 0; > > - max = vq->vring.num; > - > while ((rc = virtqueue_num_heads(vq, idx)) > 0) { > - MemoryRegionCache *desc_cache = &caches->desc; > - unsigned int num_bufs; > + unsigned int num_bufs = total_bufs; > VRingDesc desc; > unsigned int i; > > - num_bufs = total_bufs; > + desc_cache = &caches->desc; > + max = vq->vring.num; > > if (!virtqueue_get_head(vq, idx++, &i)) { > goto err; > @@ -1218,14 +1217,14 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue *vq, > wrap_counter = vq->last_avail_wrap_counter; > total_bufs = in_total = out_total = 0; > > - max = vq->vring.num; > - > for (;;) { > unsigned int num_bufs = total_bufs; > unsigned int i = idx; > int rc; > > desc_cache = &caches->desc; > + max = vq->vring.num; > + > vring_packed_desc_read(vdev, &desc, desc_cache, idx, true); > if (!is_desc_avail(desc.flags, wrap_counter)) { > break; > -- > 2.35.3 > >
On Wed, Feb 15, 2023 at 11:14:46PM +0100, Carlos López wrote: > In virtqueue_{split,packed}_get_avail_bytes() descriptors are read > in a loop via MemoryRegionCache regions and calls to > vring_{split,packed}_desc_read() - these take a region cache and the > index of the descriptor to be read. > > For direct descriptors we use a cache provided by the caller, whose > size matches that of the virtqueue vring. We limit the number of > descriptors we can read by the size of that vring: > > max = vq->vring.num; > ... > MemoryRegionCache *desc_cache = &caches->desc; > > For indirect descriptors, we initialize a new cache and limit the > number of descriptors by the size of the intermediate descriptor: > > len = address_space_cache_init(&indirect_desc_cache, > vdev->dma_as, > desc.addr, desc.len, false); > desc_cache = &indirect_desc_cache; > ... > max = desc.len / sizeof(VRingDesc); > > However, the first initialization of `max` is done outside the loop > where we process guest descriptors, while the second one is done > inside. This means that a sequence of an indirect descriptor followed > by a direct one will leave a stale value in `max`. If the second > descriptor's `next` field is smaller than the stale value, but > greater than the size of the virtqueue ring (and thus the cached > region), a failed assertion will be triggered in > address_space_read_cached() down the call chain. > > Fix this by initializing `max` inside the loop in both functions. > > Fixes: 9796d0ac8fb0 ("virtio: use address_space_map/unmap to access descriptors") > Signed-off-by: Carlos López <clopez@suse.de> > --- > hw/virtio/virtio.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index f35178f5fc..db70c4976e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -1071,6 +1071,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, > VirtIODevice *vdev = vq->vdev; > unsigned int max, idx; > unsigned int total_bufs, in_total, out_total; > + MemoryRegionCache *desc_cache; why are you moving desc_cache here? > MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; > int64_t len = 0; > int rc; > @@ -1078,15 +1079,13 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, > idx = vq->last_avail_idx; > total_bufs = in_total = out_total = 0; > > - max = vq->vring.num; > - > while ((rc = virtqueue_num_heads(vq, idx)) > 0) { > - MemoryRegionCache *desc_cache = &caches->desc; > - unsigned int num_bufs; > + unsigned int num_bufs = total_bufs; > VRingDesc desc; > unsigned int i; > > - num_bufs = total_bufs; nice cleanup but not a bugfix. Keep cleanups separate from fixes pls. > + desc_cache = &caches->desc; init as part of declaration seems cleaner. > + max = vq->vring.num; > can we move declaration of max here within the loop? will make sure the problem does not recur. > if (!virtqueue_get_head(vq, idx++, &i)) { > goto err; > @@ -1218,14 +1217,14 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue *vq, > wrap_counter = vq->last_avail_wrap_counter; > total_bufs = in_total = out_total = 0; > > - max = vq->vring.num; > - > for (;;) { > unsigned int num_bufs = total_bufs; > unsigned int i = idx; > int rc; > > desc_cache = &caches->desc; > + max = vq->vring.num; > + same question can we move declaration into the loop? > vring_packed_desc_read(vdev, &desc, desc_cache, idx, true); > if (!is_desc_avail(desc.flags, wrap_counter)) { > break; > -- > 2.35.3
Hi, On 1/3/23 23:03, Michael S. Tsirkin wrote:>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index f35178f5fc..db70c4976e 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -1071,6 +1071,7 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, >> VirtIODevice *vdev = vq->vdev; >> unsigned int max, idx; >> unsigned int total_bufs, in_total, out_total; >> + MemoryRegionCache *desc_cache; > > why are you moving desc_cache here? Just to make virtqueue_split_get_avail_bytes() and virtqueue_packed_get_avail_bytes() symmetrical, but I'll keep it out of v2. >> MemoryRegionCache indirect_desc_cache = MEMORY_REGION_CACHE_INVALID; >> int64_t len = 0; >> int rc; >> @@ -1078,15 +1079,13 @@ static void virtqueue_split_get_avail_bytes(VirtQueue *vq, >> idx = vq->last_avail_idx; >> total_bufs = in_total = out_total = 0; >> >> - max = vq->vring.num; >> - >> while ((rc = virtqueue_num_heads(vq, idx)) > 0) { >> - MemoryRegionCache *desc_cache = &caches->desc; >> - unsigned int num_bufs; >> + unsigned int num_bufs = total_bufs; >> VRingDesc desc; >> unsigned int i; >> >> - num_bufs = total_bufs; > > nice cleanup but not a bugfix. Keep cleanups separate from fixes pls. Ok makes sense. >> + desc_cache = &caches->desc; > > init as part of declaration seems cleaner. > >> + max = vq->vring.num; >> > > can we move declaration of max here within the loop? > will make sure the problem does not recur. Yes, sounds good. >> if (!virtqueue_get_head(vq, idx++, &i)) { >> goto err; >> @@ -1218,14 +1217,14 @@ static void virtqueue_packed_get_avail_bytes(VirtQueue *vq, >> wrap_counter = vq->last_avail_wrap_counter; >> total_bufs = in_total = out_total = 0; >> >> - max = vq->vring.num; >> - >> for (;;) { >> unsigned int num_bufs = total_bufs; >> unsigned int i = idx; >> int rc; >> >> desc_cache = &caches->desc; >> + max = vq->vring.num; >> + > > > same question can we move declaration into the loop? Same as above. -- Carlos López Security Engineer SUSE Software Solutions
© 2016 - 2024 Red Hat, Inc.