Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: Fix possible guest memory corruption with inflates & deflates

From: David Hildenbrand <david@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>,
	Michael Tsirkin <mst@redhat.com>,
	Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-ppc@nongnu.org, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 2/3] virtio-balloon: Fix possible guest memory corruption with inflates & deflates
Date: Wed, 6 Mar 2019 09:14:17 +0100	[thread overview]
Message-ID: <7ca90c23-9bac-0841-7ebc-364f25ab44af@redhat.com> (raw)
In-Reply-To: <20190306030601.21986-3-david@gibson.dropbear.id.au>

On 06.03.19 04:06, David Gibson wrote:
> This fixes a balloon bug with a nasty consequence - potentially
> corrupting guest memory - but which is extremely unlikely to be
> triggered in practice.
> 
> The balloon always works in 4kiB units, but the host could have a
> larger page size on certain platforms.  Since ed48c59 "virtio-balloon:
> Safely handle BALLOON_PAGE_SIZE < host page size" we've handled this
> by accumulating requests to balloon 4kiB subpages until they formed a
> full host page.  Since f6deb6d "virtio-balloon: Remove unnecessary
> MADV_WILLNEED on deflate" we essentially ignore deflate requests.
> 
> Suppose we have a host with 8kiB pages, and one host page has subpages
> A & B.  If we get this sequence of events -
> 	inflate A
> 	deflate A
> 	inflate B
> - the current logic will discard the whole host page.  That's
> incorrect because the guest has deflated subpage A, and could have
> written important data to it.
> 
> This patch fixes the problem by adjusting our state information about
> partially ballooned host pages when deflate requests are received.
> 
> Fixes: ed48c59 "virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size"
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/virtio/virtio-balloon.c | 48 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 127289ae0e..7412bf8c85 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -111,6 +111,43 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>      }
>  }
>  
> +static void balloon_deflate_page(VirtIOBalloon *balloon,
> +                                 MemoryRegion *mr, hwaddr offset)
> +{
> +    void *addr = memory_region_get_ram_ptr(mr) + offset;
> +    RAMBlock *rb;
> +    size_t rb_page_size;
> +    ram_addr_t ram_offset, host_page_base;
> +
> +    /* XXX is there a better way to get to the RAMBlock than via a
> +     * host address? */
> +    rb = qemu_ram_block_from_host(addr, false, &ram_offset);
> +    rb_page_size = qemu_ram_pagesize(rb);
> +    host_page_base = ram_offset & ~(rb_page_size - 1);
> +
> +    if (balloon->pbp
> +        && rb == balloon->pbp->rb
> +        && host_page_base == balloon->pbp->base) {
> +        int subpages = rb_page_size / BALLOON_PAGE_SIZE;
> +
> +        /*
> +         * This means the guest has asked to discard some of the 4kiB
> +         * subpages of a host page, but then changed its mind and
> +         * asked to keep them after all.  It's exceedingly unlikely
> +         * for a guest to do this in practice, but handle it anyway,
> +         * since getting it wrong could mean discarding memory the
> +         * guest is still using. */
> +        bitmap_clear(balloon->pbp->bitmap,
> +                     (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> +                     subpages);
> +
> +        if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
> +            g_free(balloon->pbp);
> +            balloon->pbp = NULL;
> +        }
> +    }
> +}
> +
>  static const char *balloon_stat_names[] = {
>     [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
>     [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
> @@ -314,8 +351,15 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>  
>              trace_virtio_balloon_handle_output(memory_region_name(section.mr),
>                                                 pa);
> -            if (!qemu_balloon_is_inhibited() && vq != s->dvq) {
> -                balloon_inflate_page(s, section.mr, section.offset_within_region);
> +            if (!qemu_balloon_is_inhibited()) {
> +                if (vq == s->ivq) {
> +                    balloon_inflate_page(s, section.mr,
> +                                         section.offset_within_region);
> +                } else if (vq == s->dvq) {
> +                    balloon_deflate_page(s, section.mr, section.offset_within_region);
> +                } else {
> +                    g_assert_not_reached();
> +                }
>              }
>              memory_region_unref(section.mr);
>          }
> 

Acked-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb