Re: [Qemu-devel] [PATCH-for-4.1] virtio-balloon: fix QEMU crashes on pagesize > BALLOON_PAGE_SIZE

[David Hildenbrand]

On 17.07.19 13:22, Michael S. Tsirkin wrote:
> 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: address@hidden #v4.0.0
>>>>>>> Cc: Stefan Hajnoczi <address@hidden>
>>>>>>> Cc: David Gibson <address@hidden>
>>>>>>> Cc: Michael S. Tsirkin <address@hidden>
>>>>>>> Cc: Igor Mammedov <address@hidden>
>>>>>>> Signed-off-by: David Hildenbrand <address@hidden>
>>>>>>> ---
>>>>>>>  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.

Right, either that or the page size, which is orthogonal.

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

If they are not dereferences, I don't consider it an ultimate problem.
But yeah, I'll look into that tomorrow. Can you pick up these patches in
the meantime?

Thanks!

-- 

Thanks,

David / dhildenb