On Wed, Jul 17, 2019 at 01:10:21PM +0200, David Hildenbrand wrote:
> On 17.07.19 13:06, Michael S. Tsirkin wrote:
> > On Wed, Jul 17, 2019 at 12:17:57PM +0200, David Hildenbrand wrote:
> >> On 17.07.19 12:04, David Hildenbrand wrote:
> >>> On 17.07.19 11:57, Michael S. Tsirkin wrote:
> >>>> On Wed, Jul 17, 2019 at 10:42:55AM +0200, David Hildenbrand wrote:
> >>>>> We are using the wrong functions to set/clear bits, effectively touching
> >>>>> multiple bits, writing out of range of the bitmap, resulting in memory
> >>>>> corruptions. We have to use set_bit()/clear_bit() instead.
> >>>>>
> >>>>> Can easily be reproduced by starting a qemu guest on hugetlbfs memory,
> >>>>> inflating the balloon. QEMU crashes. This never could have worked
> >>>>> properly - especially, also pages would have been discarded when the
> >>>>> first sub-page would be inflated (the whole bitmap would be set).
> >>>>>
> >>>>> While testing I realized, that on hugetlbfs it is pretty much impossible
> >>>>> to discard a page - the guest just frees the 4k sub-pages in random
> >>>>> order
> >>>>> most of the time. I was only able to discard a hugepage a handful of
> >>>>> times - so I hope that now works correctly.
> >>>>>
> >>>>> Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE <
> >>>>> host page size")
> >>>>> Fixes: b27b32391404 ("virtio-balloon: Fix possible guest memory
> >>>>> corruption
> >>>>> with inflates & deflates")
> >>>>> Cc: qemu-sta...@nongnu.org #v4.0.0
> >>>>> Cc: Stefan Hajnoczi <stefa...@redhat.com>
> >>>>> Cc: David Gibson <da...@gibson.dropbear.id.au>
> >>>>> Cc: Michael S. Tsirkin <m...@redhat.com>
> >>>>> Cc: Igor Mammedov <imamm...@redhat.com>
> >>>>> Signed-off-by: David Hildenbrand <da...@redhat.com>
> >>>>> ---
> >>>>> hw/virtio/virtio-balloon.c | 10 ++++------
> >>>>> 1 file changed, 4 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> >>>>> index e85d1c0d5c..669067d661 100644
> >>>>> --- a/hw/virtio/virtio-balloon.c
> >>>>> +++ b/hw/virtio/virtio-balloon.c
> >>>>> @@ -94,9 +94,8 @@ static void balloon_inflate_page(VirtIOBalloon
> >>>>> *balloon,
> >>>>> balloon->pbp->base = host_page_base;
> >>>>> }
> >>>>>
> >>>>> - bitmap_set(balloon->pbp->bitmap,
> >>>>> - (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> >>>>> - subpages);
> >>>>> + set_bit((ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> >>>>> + balloon->pbp->bitmap);
> >>>>>
> >>>>> if (bitmap_full(balloon->pbp->bitmap, subpages)) {
> >>>>> /* We've accumulated a full host page, we can actually discard
> >>>>> @@ -140,9 +139,8 @@ static void balloon_deflate_page(VirtIOBalloon
> >>>>> *balloon,
> >>>>> * 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);
> >>>>> + clear_bit((ram_offset - balloon->pbp->base) /
> >>>>> BALLOON_PAGE_SIZE,
> >>>>> + balloon->pbp->bitmap);
> >>>>>
> >>>>> if (bitmap_empty(balloon->pbp->bitmap, subpages)) {
> >>>>> g_free(balloon->pbp);
> >>>>
> >>>> I also started to wonder about this:
> >>>>
> >>>> if (!balloon->pbp) {
> >>>> /* Starting on a new host page */
> >>>> size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
> >>>> balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) +
> >>>> bitlen);
> >>>> balloon->pbp->rb = rb;
> >>>> balloon->pbp->base = host_page_base;
> >>>> }
> >>>>
> >>>> Is keeping a pointer to a ram block like this safe? what if the ramblock
> >>>> gets removed?
> >>>>
> >>>
> >>> David added
> >>>
> >>> if (balloon->pbp
> >>> && (rb != balloon->pbp->rb ) ...
> >>>
> >>> So in case the rb changes (IOW replaced - delete old one, new one
> >>> added), we reset the data.
> >>>
> >>> After a ram block was deleted, there will be no more deflation requests
> >>> coming in for it. This should be fine I guess.
> >
> > I think it might happen that an old dangling pointer happens
> > to match a newly allocated one.
> > I think we really should just cache all data we want to take into account
> > and compare that.
>
> That's true. I think just remembering and comparing the GPA base address
> would be sufficient.
Well we need to know the bitmap size allocated, too.
And I guess when we are ready to free we should
re-check it just in case.
> However, I don't consider this here to trigger easily. We would need
> some crazy memory unplug+replug going on while using the balloon. So I
> assume we can just rework this part after 4.1
Dangling pointers are just a recipe for CVEs. I'd rather rework it now.
> --
>
> Thanks,
>
> David / dhildenb
