From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by mx.groups.io with SMTP id smtpd.web11.3135.1624784756254311708 for ; Sun, 27 Jun 2021 02:05:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@ibm.com header.s=pp1 header.b=oKWP7ukk; spf=pass (domain: linux.ibm.com, ip: 148.163.156.1, mailfrom: dovmurik@linux.ibm.com) Received: from pps.filterd (m0098410.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 15R94CtH118357; Sun, 27 Jun 2021 05:05:55 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=subject : to : cc : references : from : message-id : date : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pp1; bh=idDyHOQaLk2EuugGVP5BPblx4e4VII+8WfpFD2GFRYs=; b=oKWP7ukkQoBC0xqziJgDUtbUDSoxL3RXaJGoPObiMyoHXU/Hgd4HkymfIOgsNzndE7zk k5H2xqT0u18NV5NlAa2cVDdRZnwUR22aIvYKtpLbGr/zk/o5TxIL2bZlrHbfA2mqWsHh +xnXggM41EZODD2IdfOe0Deh5fMliZfr4wjp3PK+1KJQXoB3Skqf6EbcA/oL3NJElulg peXdSoUxGrvpgj6MoLFf+Uo5W9iTnLYZjoXTHx4p2+oxaO3Vsf6sgXxyONpMs5gF2bNz vSMtUB+VnZgdSJK/b1DogQx/YhUqoHJyWnilsF2f+Kxnh4r6VtCYXgHtTM2Ki7ikA3PO pQ== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 39ep3c09c1-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 27 Jun 2021 05:05:55 -0400 Received: from m0098410.ppops.net (m0098410.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.43/8.16.0.43) with SMTP id 15R953FS120137; Sun, 27 Jun 2021 05:05:55 -0400 Received: from ppma01fra.de.ibm.com (46.49.7a9f.ip4.static.sl-reverse.com [159.122.73.70]) by mx0a-001b2d01.pphosted.com with ESMTP id 39ep3c09ba-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 27 Jun 2021 05:05:54 -0400 Received: from pps.filterd (ppma01fra.de.ibm.com [127.0.0.1]) by ppma01fra.de.ibm.com (8.16.1.2/8.16.1.2) with SMTP id 15R95qlq009578; Sun, 27 Jun 2021 09:05:52 GMT Received: from b06cxnps4076.portsmouth.uk.ibm.com (d06relay13.portsmouth.uk.ibm.com [9.149.109.198]) by ppma01fra.de.ibm.com with ESMTP id 39duv8861s-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 27 Jun 2021 09:05:52 +0000 Received: from b06wcsmtp001.portsmouth.uk.ibm.com (b06wcsmtp001.portsmouth.uk.ibm.com [9.149.105.160]) by b06cxnps4076.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 15R95no533882482 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sun, 27 Jun 2021 09:05:49 GMT Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 61582A4065; Sun, 27 Jun 2021 09:05:49 +0000 (GMT) Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 376D2A405F; Sun, 27 Jun 2021 09:05:45 +0000 (GMT) Received: from [9.160.49.135] (unknown [9.160.49.135]) by b06wcsmtp001.portsmouth.uk.ibm.com (Postfix) with ESMTP; Sun, 27 Jun 2021 09:05:44 +0000 (GMT) Subject: Re: [edk2-devel] [PATCH v2 2/3] OvmfPkg/GenericQemuLoadImageLib: Read cmdline from QemuKernelLoaderFs To: Laszlo Ersek , devel@edk2.groups.io Cc: Ard Biesheuvel , Jordan Justen , James Bottomley , Tobin Feldman-Fitzthum , Dov Murik References: <20210617121700.2883073-1-dovmurik@linux.ibm.com> <20210617121700.2883073-3-dovmurik@linux.ibm.com> From: "Dov Murik" Message-ID: <96ffbd2a-301d-b13a-3b83-af16b40228b9@linux.ibm.com> Date: Sun, 27 Jun 2021 12:05:43 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 In-Reply-To: X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: YoIw7mUmiPLW_-RM75oM6ET4qJhJDD4L X-Proofpoint-GUID: 2NkloX2d_P2Mn19JIFKc6_lmRM6UIJ6u X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790 definitions=2021-06-26_15:2021-06-25,2021-06-26 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 phishscore=0 impostorscore=0 suspectscore=0 bulkscore=0 spamscore=0 clxscore=1015 mlxscore=0 adultscore=0 lowpriorityscore=0 mlxlogscore=999 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2106270064 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, On 24/06/2021 20:49, Laszlo Ersek wrote: > Hi Dov, > > On 06/17/21 14:16, Dov Murik wrote: >> Remove the QemuFwCfgLib interface used to read the QEMU cmdline >> (-append argument) and the initrd size. Instead, use the synthetic >> filesystem QemuKernelLoaderFs which has three files: "kernel", "initrd", >> and "cmdline". >> >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: James Bottomley >> Cc: Tobin Feldman-Fitzthum >> Signed-off-by: Dov Murik >> --- >> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf | 2 +- >> OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c | 145 ++++++++++++++++++-- >> 2 files changed, 133 insertions(+), 14 deletions(-) > > This update seems to address everything that Ard requested under v1; > thanks. > > My comments: > > (1) I spent a lot of time reviewing your patch. Unfortunately, I found a > preexistent bug in both QemuLoadImageLib instances, which we should fix > first, in two separate patches. > > The bug was introduced in commit ddd2be6b0026 ("OvmfPkg: provide a > generic implementation of QemuLoadImageLib", 2020-03-05). Unfortunately > I missed the bug in my original review. > > In said commit, the QemuLoadKernelImage() function > [OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c] > refactored / reimplemented the logic from the TryRunningQemuKernel() > function [ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c]. > > If we now check out the tree at ddd2be6b0026, and compare the above two > functions, we notice the following: > > (1a) TryRunningQemuKernel() downloads all three blobs via fw_cfg in the > beginning, and *always* frees all successfully downloaded blobs at the > end, under the "FreeBlobs" label. > > (1b) In QemuLoadKernelImage(), the kernel and initrd fw_cfg blobs are > owned by the QemuKernelLoaderFsDxe driver; only the command line blob is > downloaded from fw_cfg. Not freeing the former two blobs (kernel and > initrd) makes sense. *However*, the command line blob should *still* be > freed, even if QemuLoadKernelImage() succeeds! That's because we have no > use for the command line fw_cfg blob, after it is translated to > LoadOptions. > > The bug is that QemuLoadKernelImage() leaks "CommandLine" on success. > > > The same issue was introduced in the other lib instance > [OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.c], in commit > 7c47d89003a6 ("OvmfPkg: implement QEMU loader library for X86 with > legacy fallback", 2020-03-05). > > > The fix is identical between both library instances: > >> @@ -193,14 +193,16 @@ QemuLoadKernelImage ( >> } >> >> *ImageHandle = KernelImageHandle; >> - return EFI_SUCCESS; >> + Status = EFI_SUCCESS; >> >> FreeCommandLine: >> if (CommandLineSize > 0) { >> FreePool (CommandLine); >> } >> UnloadImage: >> - gBS->UnloadImage (KernelImageHandle); >> + if (EFI_ERROR (Status)) { >> + gBS->UnloadImage (KernelImageHandle); >> + } >> >> return Status; >> } > > Can you please submit this fix twice, in two separate patches at the > *very front* of this series, one patch for each lib instance? Something > like: > > #1 OvmfPkg/GenericQemuLoadImageLib: plug cmdline blob leak on success > ... > Reported-by: Laszlo Ersek > Fixes: ddd2be6b0026abcd0f819b3915fc80c3de81dd62 > > #2 OvmfPkg/X86QemuLoadImageLib: plug cmdline blob leak on success > ... > Reported-by: Laszlo Ersek > Fixes: 7c47d89003a6f8f7f6f0ce8ca7d3e87c630d14cc > > Thank you in advance! OK, I'll add these two patches. > > Then, comments on your actual patch: > > (2) The bugzilla ticket should be referenced in the commit message > please, above your signoff: > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3457 > OK, I'll add it. > > On 06/17/21 14:16, Dov Murik wrote: >> >> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf >> index b262cb926a4d..f462fd6922cf 100644 >> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf >> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf >> @@ -27,12 +27,12 @@ [LibraryClasses] >> DebugLib >> MemoryAllocationLib >> PrintLib >> - QemuFwCfgLib >> UefiBootServicesTableLib >> >> [Protocols] >> gEfiDevicePathProtocolGuid >> gEfiLoadedImageProtocolGuid >> + gEfiSimpleFileSystemProtocolGuid >> >> [Guids] >> gQemuKernelLoaderFsMediaGuid > > (3) The FileHandleLib class should be added, under [LibraryClasses]. > OK. > >> diff --git a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c >> index 114db7e8441f..f520456e3b24 100644 >> --- a/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c >> +++ b/OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.c >> @@ -11,9 +11,9 @@ >> #include >> #include >> #include >> +#include >> #include >> #include >> -#include >> #include >> #include >> #include > > (4) The new "gEfiSimpleFileSystemProtocolGuid" dependency should be > reflected here too, by adding: > > #include > > (In general the [Protocols] section of the INF file should be matched by > #include directives.) > > This was masked from you because pulled in > , but that's not enough justification for a > difference between the INF [Protocols] section and the #include > directive list. I'll make sure the #include section matches the [Protocols]. > > >> @@ -30,6 +30,11 @@ typedef struct { >> KERNEL_FILE_DEVPATH FileNode; >> EFI_DEVICE_PATH_PROTOCOL EndNode; >> } KERNEL_VENMEDIA_FILE_DEVPATH; >> + >> +typedef struct { >> + VENDOR_DEVICE_PATH VenMediaNode; >> + EFI_DEVICE_PATH_PROTOCOL EndNode; >> +} SINGLE_VENMEDIA_NODE_DEVPATH; >> #pragma pack () >> >> STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = { >> @@ -51,6 +56,78 @@ STATIC CONST KERNEL_VENMEDIA_FILE_DEVPATH mKernelDevicePath = { >> } >> }; >> >> +STATIC CONST SINGLE_VENMEDIA_NODE_DEVPATH mQemuKernelLoaderFileSystemDevicePath = { > > (5) This variable name causes two overlong lines in the file; it should > be renamed to "mQemuKernelLoaderFsDevicePath" please. > Good idea, I'll rename. > >> + { >> + { >> + MEDIA_DEVICE_PATH, MEDIA_VENDOR_DP, >> + { sizeof (VENDOR_DEVICE_PATH) } >> + }, >> + QEMU_KERNEL_LOADER_FS_MEDIA_GUID >> + }, { >> + END_DEVICE_PATH_TYPE, END_ENTIRE_DEVICE_PATH_SUBTYPE, >> + { sizeof (EFI_DEVICE_PATH_PROTOCOL) } >> + } >> +}; >> + >> +STATIC >> +EFI_STATUS >> +GetQemuKernelLoaderBlobSize ( >> + IN EFI_FILE_HANDLE Root, >> + IN CHAR16 *FileName, >> + OUT UINTN *Size >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_FILE_HANDLE FileHandle; >> + UINT64 FileSize; >> + >> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + Status = FileHandleGetSize (FileHandle, &FileSize); >> + if (EFI_ERROR (Status)) { >> + goto CloseFile; >> + } >> + *Size = FileSize; > > (6) Silent truncation from UINT64 to UINTN, even if theoretical, is bad > practice. Please do this: > > if (FileSize > MAX_UINTN) { > Status = EFI_UNSUPPORTED; > goto CloseFile; > } > *Size = (UINTN)FileSize; > OK. > >> + Status = EFI_SUCCESS; >> +CloseFile: >> + FileHandle->Close (FileHandle); >> + return Status; >> +} >> + >> +STATIC >> +EFI_STATUS >> +ReadWholeQemuKernelLoaderBlob ( >> + IN EFI_FILE_HANDLE Root, >> + IN CHAR16 *FileName, >> + IN UINTN Size, >> + OUT VOID *Buffer >> + ) >> +{ >> + EFI_STATUS Status; >> + EFI_FILE_HANDLE FileHandle; >> + UINTN ReadSize; >> + >> + Status = Root->Open (Root, &FileHandle, FileName, EFI_FILE_MODE_READ, 0); >> + if (EFI_ERROR (Status)) { >> + return Status; >> + } >> + ReadSize = Size; >> + Status = FileHandle->Read (FileHandle, &ReadSize, Buffer); >> + if (EFI_ERROR (Status)) { >> + goto CloseFile; >> + } >> + if (ReadSize != Size) { >> + Status = EFI_PROTOCOL_ERROR; >> + goto CloseFile; >> + } >> + Status = EFI_SUCCESS; >> +CloseFile: >> + FileHandle->Close (FileHandle); >> + return Status; >> +} >> + >> /** >> Download the kernel, the initial ramdisk, and the kernel command line from >> QEMU's fw_cfg. The kernel will be instructed via its command line to load >> @@ -76,12 +153,16 @@ QemuLoadKernelImage ( >> OUT EFI_HANDLE *ImageHandle >> ) >> { >> - EFI_STATUS Status; >> - EFI_HANDLE KernelImageHandle; >> - EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage; >> - UINTN CommandLineSize; >> - CHAR8 *CommandLine; >> - UINTN InitrdSize; >> + EFI_STATUS Status; >> + EFI_HANDLE KernelImageHandle; >> + EFI_LOADED_IMAGE_PROTOCOL *KernelLoadedImage; >> + EFI_DEVICE_PATH_PROTOCOL *DevicePathNode; >> + EFI_HANDLE FsVolumeHandle; >> + EFI_SIMPLE_FILE_SYSTEM_PROTOCOL *FsProtocol; >> + EFI_FILE_HANDLE Root; >> + UINTN CommandLineSize; >> + CHAR8 *CommandLine; >> + UINTN InitrdSize; >> >> // >> // Load the image. This should call back into the QEMU EFI loader file system. >> @@ -124,8 +205,38 @@ QemuLoadKernelImage ( >> ); >> ASSERT_EFI_ERROR (Status); >> >> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineSize); >> - CommandLineSize = (UINTN)QemuFwCfgRead32 (); >> + // >> + // Open the Qemu Kernel Loader abstract filesystem (volume) which will be >> + // used to read the "initrd" and "cmdline" synthetic files. >> + // > > (7) This comment is welcome, but it is inexact. > > We'll use the filesystem for reading the command line, yes, but > regarding the initrd, we use the filesystem only for learning the *size* > of the initrd. (And even the size of the initrd is only interesting > inasmuch a nonzero size means that an initrd is *present*.) The initrd > blob itself is not read by us. > > I suggest: > > used to query the "initrd" and to read the "cmdline" synthetic files. > You're right. I wrote correctly in the commit message but not accurately in this code comment. I'll update. > >> + DevicePathNode = (EFI_DEVICE_PATH_PROTOCOL *)&mQemuKernelLoaderFileSystemDevicePath; >> + Status = gBS->LocateDevicePath ( >> + &gEfiSimpleFileSystemProtocolGuid, >> + &DevicePathNode, >> + &FsVolumeHandle >> + ); >> + if (EFI_ERROR (Status)) { >> + return Status; > > (8) This leaks "KernelImageHandle". At this point, gBS->LoadImage() at > the top of the function will have succeeded. > > Please jump to the UnloadImage label, rather than returning. > OK. > >> + } >> + >> + Status = gBS->HandleProtocol ( >> + FsVolumeHandle, >> + &gEfiSimpleFileSystemProtocolGuid, >> + (VOID **)&FsProtocol >> + ); >> + if (EFI_ERROR (Status)) { >> + return Status; > > (9) Same leak as described in (8); please jump to the UnloadImage label. > OK. > >> + } >> + >> + Status = FsProtocol->OpenVolume (FsVolumeHandle, &Root); >> + if (EFI_ERROR (Status)) { >> + return Status; > > (10) Same leak as described in (8); please jump to the UnloadImage > label. > OK. > >> + } >> + >> + Status = GetQemuKernelLoaderBlobSize (Root, L"cmdline", &CommandLineSize); >> + if (EFI_ERROR (Status)) { >> + goto CloseRoot; >> + } >> >> if (CommandLineSize == 0) { >> KernelLoadedImage->LoadOptionsSize = 0; >> @@ -136,8 +247,11 @@ QemuLoadKernelImage ( >> goto UnloadImage; >> } > > (11) Not fully shown in the context, but here we have: > > if (CommandLineSize == 0) { > KernelLoadedImage->LoadOptionsSize = 0; > } else { > CommandLine = AllocatePool (CommandLineSize); > if (CommandLine == NULL) { > Status = EFI_OUT_OF_RESOURCES; > goto UnloadImage; > } > > Note that we have a "goto UnloadImage" in it. > > Please update that to "goto CloseRoot". > Yes, missed that. Thanks for catching it. I'll fix. > >> >> - QemuFwCfgSelectItem (QemuFwCfgItemCommandLineData); >> - QemuFwCfgReadBytes (CommandLineSize, CommandLine); >> + Status = ReadWholeQemuKernelLoaderBlob (Root, L"cmdline", CommandLineSize, >> + CommandLine); >> + if (EFI_ERROR (Status)) { >> + goto FreeCommandLine; >> + } >> >> // >> // Verify NUL-termination of the command line. >> @@ -155,8 +269,10 @@ QemuLoadKernelImage ( >> KernelLoadedImage->LoadOptionsSize = (UINT32)((CommandLineSize - 1) * 2); >> } >> >> - QemuFwCfgSelectItem (QemuFwCfgItemInitrdSize); >> - InitrdSize = (UINTN)QemuFwCfgRead32 (); >> + Status = GetQemuKernelLoaderBlobSize (Root, L"initrd", &InitrdSize); >> + if (EFI_ERROR (Status)) { >> + goto FreeCommandLine; >> + } >> >> if (InitrdSize > 0) { >> // >> @@ -193,6 +309,7 @@ QemuLoadKernelImage ( >> } >> >> *ImageHandle = KernelImageHandle; >> + Root->Close (Root); >> return EFI_SUCCESS; >> >> FreeCommandLine: >> @@ -201,6 +318,8 @@ FreeCommandLine: >> } >> UnloadImage: >> gBS->UnloadImage (KernelImageHandle); >> +CloseRoot: >> + Root->Close (Root); >> >> return Status; >> } >> > > (12) So, the order of handlers is incorrect here, and when I looked into > it, that was when I actually found preexistent issue (1). > > The desired epilogue for the function is: > >> *ImageHandle = KernelImageHandle; >> Status = EFI_SUCCESS; >> >> FreeCommandLine: >> if (CommandLineSize > 0) { >> FreePool (CommandLine); >> } >> CloseRoot: >> Root->Close (Root); >> UnloadImage: >> if (EFI_ERROR (Status)) { >> gBS->UnloadImage (KernelImageHandle); >> } >> >> return Status; > > The idea is that CommandLine and Root are both temporaries, and as such > they need to be released on either success or failure. Whereas > KernelImageHandle must be released precisely on failure. Furthermore, in > either case, they must cascade as shown above -- in reverse order of > construction. OK, I'll modify this. Thanks, -Dov