[PATCH] virtio: fix reachable assertion due to stale value of cached region size

[PATCH] virtio: fix reachable assertion due to stale value of cached region size

[Carlos López]

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

Re: [PATCH] virtio: fix reachable assertion due to stale value of cached region size

[Michael S. Tsirkin]

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

Re: [PATCH] virtio: fix reachable assertion due to stale value of cached region size

[Carlos López]

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

Re: [PATCH] virtio: fix reachable assertion due to stale value of cached region size

[Jason Wang]

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