virtio: add check for descriptor's mapped address

Commit Message

Prasad Pandit Sept. 15, 2016, 11:34 a.m. UTC

From: Prasad J Pandit <[e-mail]>

virtio back end uses set of buffers to facilitate I/O operations.
If its size is too large, 'cpu_physical_memory_map' could return
a null address. This would result in a null dereference
while un-mapping descriptors. Add check to avoid it.

Reported-by: Qinghao Tang <[e-mail]>
Signed-off-by: Prasad J Pandit <[e-mail]>
---
 hw/virtio/virtio.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Laszlo Ersek Sept. 16, 2016, 5 p.m. UTC | #1

CC Stefan

On 09/15/16 13:34, P J P wrote:
> From: Prasad J Pandit <[e-mail]>
> 
> virtio back end uses set of buffers to facilitate I/O operations.
> If its size is too large, 'cpu_physical_memory_map' could return
> a null address. This would result in a null dereference
> while un-mapping descriptors. Add check to avoid it.
> 
> Reported-by: Qinghao Tang <[e-mail]>
> Signed-off-by: Prasad J Pandit <[e-mail]>
> ---
>  hw/virtio/virtio.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 15ee3a7..0a4c5b6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -472,12 +472,14 @@ static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
>          }
>  
>          iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
> -        iov[num_sg].iov_len = len;
> -        addr[num_sg] = pa;
> +        if (iov[num_sg].iov_base) {
> +            iov[num_sg].iov_len = len;
> +            addr[num_sg] = pa;
>  
> +            pa += len;
> +            num_sg++;
> +        }
>          sz -= len;
> -        pa += len;
> -        num_sg++;
>      }
>      *p_num_sg = num_sg;
>  }
> 

I think the situation you describe is a guest bug. Just above the code
that you modify, there's already

    if (!sz) {
        error_report("virtio: zero sized buffers are not allowed");
        exit(1);
    }

I think it would be reasonable to handle this other guest bug similarly:

    if (iov[num_sg].iov_base == NULL) {
        error_report("virtio: bogus descriptor or out of resources");
        exit(EXIT_FAILURE);
    }

The main message is of course "bogus descriptor". OTOH,
cpu_physical_memory_map() / address_space_map() can return NULL for
multiple reasons, not all of which seem guest errors: the loop in
virtqueue_map_desc() handles the case when cpu_physical_memory_map()
cannot map the entire area requested by the descriptor in a single go,
and then mapping (part) of the remaining area might fail due to resource
exhaustion in QEMU (see "resources needed to perform the mapping are
exhausted" on address_space_map()).

So maybe those error returns from address_space_map() should be
disentangled first. (Although, the only difference they'd make at this
point would be in the error message when we bail out anyway.)

So, unless Stefan or someone else has a better idea, I suggest the above
error message, and exit(EXIT_FAILURE). Silently skipping a part (or all
remaining parts) of the area requested by the descriptor is unlikely to
produce predictable results for the guest (and the user).

Thanks
Laszlo

Prasad Pandit Sept. 19, 2016, 9:07 a.m. UTC | #2

+-- On Fri, 16 Sep 2016, Laszlo Ersek wrote --+
| CC Stefan
| I think it would be reasonable to handle this other guest bug similarly:
| 
|     if (iov[num_sg].iov_base == NULL) {
|         error_report("virtio: bogus descriptor or out of resources");
|         exit(EXIT_FAILURE);
|     }
...
| So, unless Stefan or someone else has a better idea, I suggest the above
| error message, and exit(EXIT_FAILURE).

I have sent a revised patch v2. Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Patch

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 15ee3a7..0a4c5b6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -472,12 +472,14 @@  static void virtqueue_map_desc(unsigned int *p_num_sg, hwaddr *addr, struct iove
         }
 
         iov[num_sg].iov_base = cpu_physical_memory_map(pa, &len, is_write);
-        iov[num_sg].iov_len = len;
-        addr[num_sg] = pa;
+        if (iov[num_sg].iov_base) {
+            iov[num_sg].iov_len = len;
+            addr[num_sg] = pa;
 
+            pa += len;
+            num_sg++;
+        }
         sz -= len;
-        pa += len;
-        num_sg++;
     }
     *p_num_sg = num_sg;
 }