Re: [PATCH] hw/display/ramfb: plug slight guest-triggerable leak on mode setting

[Laszlo Ersek]

On 9/19/23 15:19, Laszlo Ersek wrote:
> The fw_cfg DMA write callback in ramfb prepares a new display surface in
> QEMU; this new surface is put to use ("swapped in") upon the next display
> update. At that time, the old surface (if any) is released.
> 
> If the guest triggers the fw_cfg DMA write callback at least twice between
> two adjacent display updates, then the second callback (and further such
> callbacks) will leak the previously prepared (but not yet swapped in)
> display surface.
> 
> The issue can be shown by:
> 
> (1) starting QEMU with "-trace displaysurface_free", and
> 
> (2) running the following program in the guest UEFI shell:
> 
>> #include <Library/ShellCEntryLib.h>           // ShellAppMain()
>> #include <Library/UefiBootServicesTableLib.h> // gBS
>> #include <Protocol/GraphicsOutput.h>          // EFI_GRAPHICS_OUTPUT_PROTOCOL
>>
>> INTN
>> EFIAPI
>> ShellAppMain (
>>   IN UINTN   Argc,
>>   IN CHAR16  **Argv
>>   )
>> {
>>   EFI_STATUS                    Status;
>>   VOID                          *Interface;
>>   EFI_GRAPHICS_OUTPUT_PROTOCOL  *Gop;
>>   UINT32                        Mode;
>>
>>   Status = gBS->LocateProtocol (
>>                   &gEfiGraphicsOutputProtocolGuid,
>>                   NULL,
>>                   &Interface
>>                   );
>>   if (EFI_ERROR (Status)) {
>>     return 1;
>>   }
>>
>>   Gop = Interface;
>>
>>   Mode = 1;
>>   for ( ; ;) {
>>     Status = Gop->SetMode (Gop, Mode);
>>     if (EFI_ERROR (Status)) {
>>       break;
>>     }
>>
>>     Mode = 1 - Mode;
>>   }
>>
>>   return 1;
>> }
> 
> The symptom is then that:
> 
> - only one trace message appears periodically,
> 
> - the time between adjacent messages keeps increasing -- implying that
>   some list structure (containing the leaked resources) keeps growing,
> 
> - the "surface" pointer is ever different.
> 
>> 18566@1695127471.449586:displaysurface_free surface=0x7f2fcc09a7c0
>> 18566@1695127471.529559:displaysurface_free surface=0x7f2fcc9dac10
>> 18566@1695127471.659812:displaysurface_free surface=0x7f2fcc441dd0
>> 18566@1695127471.839669:displaysurface_free surface=0x7f2fcc0363d0
>> 18566@1695127472.069674:displaysurface_free surface=0x7f2fcc413a80
>> 18566@1695127472.349580:displaysurface_free surface=0x7f2fcc09cd00
>> 18566@1695127472.679783:displaysurface_free surface=0x7f2fcc1395f0
>> 18566@1695127473.059848:displaysurface_free surface=0x7f2fcc1cae50
>> 18566@1695127473.489724:displaysurface_free surface=0x7f2fcc42fc50
>> 18566@1695127473.969791:displaysurface_free surface=0x7f2fcc45dcc0
>> 18566@1695127474.499708:displaysurface_free surface=0x7f2fcc70b9d0
>> 18566@1695127475.079769:displaysurface_free surface=0x7f2fcc82acc0
>> 18566@1695127475.709941:displaysurface_free surface=0x7f2fcc369c00
>> 18566@1695127476.389619:displaysurface_free surface=0x7f2fcc32b910
>> 18566@1695127477.119772:displaysurface_free surface=0x7f2fcc0d5a20
>> 18566@1695127477.899517:displaysurface_free surface=0x7f2fcc086c40
>> 18566@1695127478.729962:displaysurface_free surface=0x7f2fccc72020
>> 18566@1695127479.609839:displaysurface_free surface=0x7f2fcc185160
>> 18566@1695127480.539688:displaysurface_free surface=0x7f2fcc23a7e0
>> 18566@1695127481.519759:displaysurface_free surface=0x7f2fcc3ec870
>> 18566@1695127482.549930:displaysurface_free surface=0x7f2fcc634960
>> 18566@1695127483.629661:displaysurface_free surface=0x7f2fcc26b140
>> 18566@1695127484.759987:displaysurface_free surface=0x7f2fcc321700
>> 18566@1695127485.940289:displaysurface_free surface=0x7f2fccaad100
> 
> We figured this wasn't a CVE-worthy problem, as only small amounts of
> memory were leaked (the framebuffer itself is mapped from guest RAM, QEMU
> only allocates administrative structures), plus libvirt restricts QEMU
> memory footprint anyway, thus the guest can only DoS itself.
> 
> Plug the leak, by releasing the last prepared (not yet swapped in) display
> surface, if any, in the fw_cfg DMA write callback.
> 
> Regarding the "reproducer", with the fix in place, the log is flooded with
> trace messages (one per fw_cfg write), *and* the trace message alternates
> between just two "surface" pointer values (i.e., nothing is leaked, the
> allocator flip-flops between two objects in effect).
> 
> This issue appears to date back to the introducion of ramfb (995b30179bdc,
> "hw/display: add ramfb, a simple boot framebuffer living in guest ram",
> 2018-06-18).
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com> (maintainer:ramfb)
> Cc: qemu-stable@nongnu.org
> Fixes: 995b30179bdc
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  hw/display/ramfb.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/display/ramfb.c b/hw/display/ramfb.c
> index 79b9754a5820..c2b002d53480 100644
> --- a/hw/display/ramfb.c
> +++ b/hw/display/ramfb.c
> @@ -97,6 +97,7 @@ static void ramfb_fw_cfg_write(void *dev, off_t offset, 
> size_t len)
>  
>      s->width = width;
>      s->height = height;
> +    qemu_free_displaysurface(s->ds);
>      s->ds = surface;
>  }
>  

Ping.

Laszlo