Re: [PATCH 1/2] virtio-balloon: Prevent guest from starting a report when we didn't request one

[David Hildenbrand <david@redhat.com>]

On 19.06.20 23:53, Alexander Duyck wrote:
> From: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> 
> Based on code review it appears possible for the driver to force the device
> out of a stopped state when hinting by repeating the last ID it was
> provided.

Indeed, thanks for noticing.

> 
> Prevent this by only allowing a transition to the start state when we are
> in the requested state. This way the driver is only allowed to send one
> descriptor that will transition the device into the start state. All others
> will leave it in the stop state once it has finished.
> 
> In addition add the necessary locking to provent any potential races

s/provent/prevent/

> between the accesses of the cmd_id and the status.
> 
> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
> Signed-off-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
> ---
>  hw/virtio/virtio-balloon.c |   11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 10507b2a430a..7f3af266f674 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -527,7 +527,8 @@ static bool get_free_page_hints(VirtIOBalloon *dev)
>              ret = false;
>              goto out;
>          }
> -        if (id == dev->free_page_report_cmd_id) {
> +        if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED &&
> +            id == dev->free_page_report_cmd_id) {
>              dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
>          } else {
>              /*

But doesn't that mean that, after the first hint, all further ones will
be discarded and we'll enter the STOP state in the else case? Or am I
missing something?

Shouldn't this be something like

if (id == dev->free_page_report_cmd_id) {
    if (dev->free_page_report_status == FREE_PAGE_REPORT_S_REQUESTED) {
        dev->free_page_report_status = FREE_PAGE_REPORT_S_START;
    }
    /* Stay in FREE_PAGE_REPORT_S_START as long as the cmd_id match .*/
} else { ...

> @@ -592,14 +593,16 @@ static void virtio_balloon_free_page_start(VirtIOBalloon *s)
>          return;
>      }
>  
> -    if (s->free_page_report_cmd_id == UINT_MAX) {
> +    qemu_mutex_lock(&s->free_page_lock);
> +
> +    if (s->free_page_report_cmd_id++ == UINT_MAX) {
>          s->free_page_report_cmd_id =
>                         VIRTIO_BALLOON_FREE_PAGE_REPORT_CMD_ID_MIN;
> -    } else {
> -        s->free_page_report_cmd_id++;
>      }

Somewhat unrelated cleanup.

>  
>      s->free_page_report_status = FREE_PAGE_REPORT_S_REQUESTED;
> +    qemu_mutex_unlock(&s->free_page_lock);
> +
>      virtio_notify_config(vdev);
>  }
>  
> 


-- 
Thanks,

David / dhildenb