From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 41623E748F0 for ; Sun, 1 Oct 2023 00:21:45 +0000 (UTC) Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qmkCh-0004QI-Ea; Sat, 30 Sep 2023 20:20:59 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qmkCb-0004Q8-TC for qemu-devel@nongnu.org; Sat, 30 Sep 2023 20:20:53 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qmkCZ-0005f4-LD for qemu-devel@nongnu.org; Sat, 30 Sep 2023 20:20:53 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1696119650; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=XpA/gpaJHwXiIxeL7Lli/ZSj/DJXv+fETMjY2ZB1lvQ=; b=XjFHdcxo+geJFmESyhsjdPuikREQrk3dhbfei7psrsL2opKBtKu6g0q+OV+n6ScS1ehxlL Z9v48cWaZWfZ0Tyr8SNQ/OndkKfiGCM4qgkEb2hid2yHG8oZA2oDZt3hbyIA6g0H+AaLjO EWIErAgLBYd/ta2AIvwoQFfMYr5Ythw= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-452-v7kMoFeHMjqY69l7RMwx8A-1; Sat, 30 Sep 2023 20:20:47 -0400 X-MC-Unique: v7kMoFeHMjqY69l7RMwx8A-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 3EBC8185A78E; Sun, 1 Oct 2023 00:20:47 +0000 (UTC) Received: from [10.39.192.54] (unknown [10.39.192.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 49618175B5; Sun, 1 Oct 2023 00:20:46 +0000 (UTC) Message-ID: Date: Sun, 1 Oct 2023 02:20:45 +0200 MIME-Version: 1.0 Subject: Re: [PATCH] hw/display/ramfb: plug slight guest-triggerable leak on mode setting Content-Language: en-US From: Laszlo Ersek To: =?UTF-8?Q?Marc-Andr=c3=a9_Lureau?= Cc: qemu-devel@nongnu.org, Gerd Hoffmann , qemu-stable@nongnu.org References: <20230919131955.27223-1-lersek@redhat.com> <7ee37181-526e-2b6a-6b42-09340a9e70a9@redhat.com> <4da39e48-c916-ed62-70ae-f0306845f01b@redhat.com> In-Reply-To: <4da39e48-c916-ed62-70ae-f0306845f01b@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 Received-SPF: pass client-ip=170.10.133.124; envelope-from=lersek@redhat.com; helo=us-smtp-delivery-124.mimecast.com X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org On 10/1/23 00:14, Laszlo Ersek wrote: > On 9/29/23 13:17, Marc-André Lureau wrote: >> Hi >> >> On Wed, Sep 27, 2023 at 7:46 PM Laszlo Ersek wrote: >>> >>> 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 // ShellAppMain() >>>>> #include // gBS >>>>> #include // 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 (maintainer:ramfb) >>>> Cc: qemu-stable@nongnu.org >>>> Fixes: 995b30179bdc >>>> Signed-off-by: Laszlo Ersek >>>> --- >>>> 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; >>>> } >> >> Reviewed-by: Marc-André Lureau >> >> Incidentally I found the same issue: >> https://patchew.org/QEMU/20230920082634.3349487-1-marcandre.lureau@redhat.com/ > > Which patch is better? > > I certainly didn't think of g_clear_pointer(); is that more idiomatic? > >> >> >> fwiw, my migration support patch is still unreviewed: >> https://patchew.org/QEMU/20230920082651.3349712-1-marcandre.lureau@redhat.com/ >> > > I don't have a copy of that patch (not subscribed, sorry...), but how > simply you did it surprises me. I did expect to simulate an fw_cfg write > in post_load, but I thought we'd approach the device (for the sake of > including it in the migration stream) from the higher level device's > vmstate descriptors (dc->vmsd) that set up / depend on ramfb in the > first place. I didn't know that raw vmstate_register() was still accepted. > > If it is, then, for that patch (with Gerd's comment addressed): > > Acked-by: Laszlo Ersek I think I may have found one issue with that patch. The fields that we save into the migration stream are integer members of the RAMFBCfg structure that lives directly in the fw_cfg file. The ramfb device specifies those fields for the guest as big endian. This means that when ramfb_fw_cfg_write() runs, triggered by the guest, then on big endian hosts, be32_to_cpu() and be64_to_cpu() will be no-ops, as the integer representation inside the fw_cfg file will match the host CPU at once. And on little endian hosts, these functions will byte swap. In both cases, ramfb_create_display_surface() will have to be called with identical host-side integers. This means that *before* the be32_to_cpu() and be64_to_cpu() calls, the host side *values* read out from the fw_cfg file members *differ* between big endian and little endian hosts. And the problem is that we write precisely those values into the migration stream, via "vmstate_ramfb_cfg". The migration stream represents integers in big endian regardless of host endianness, but the question is the *values* that we encode in BE for the stream. And the values (from fw_cg) will differ between little endian and big endian hosts. Thus, I think we should just use VMSTATE_BUFFER(cfg, RAMFBState) in "vmstate_ramfb", and remove "vmstate_ramfb_cfg". For migration purposes, we should treat the fw_cfg file as an opaque blob. Laszlo > > BTW: can you please assign > to yourself and > link your patch into it? The reason we ended up duplicating work here is > that noone took RHBZ 1859424 before. > > ... Well, the ticket is RHEL-7478 in JIRA now, in fact. :/ > > Thanks! > Laszlo