From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web11.12304.1592318747126671799 for ; Tue, 16 Jun 2020 07:45:47 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=R4ym0EFE; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1592318746; 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=yF4wIszcS7RLEl5a06rQXMYkb2A/P0FRGw+9U4Vox/Y=; b=R4ym0EFEpM1oUS0ApJyb2ZF15ZiOcy3LCW8787Zi5XmssoUFRaSqU+0ogmtVa7djhbmWEl mLtfzc2IDEaZ+4v91dGOwgehtRTXjISWNGAwlZ5RBgK2JQtCXQxqjKKNwM7e5hVaWxNZcw 9XIloH52qMmLlSARF5ZJylcRB6XYu6A= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-347-cSoPlHeIPYGo6WBN5yRXzA-1; Tue, 16 Jun 2020 10:45:37 -0400 X-MC-Unique: cSoPlHeIPYGo6WBN5yRXzA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id E853318A8222; Tue, 16 Jun 2020 14:45:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-248.ams2.redhat.com [10.36.113.248]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6AF157CAB7; Tue, 16 Jun 2020 14:45:31 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 2/5] MdeModulePkg/SmmCore: Remove UEFI LOADED_IMAGE from SMM. To: devel@edk2.groups.io, zhiguang.liu@intel.com Cc: Feng Tian , Star Zeng , Michael D Kinney , Jiewen Yao , Eric Dong , Hao A Wu , Jian J Wang , Ray Ni References: <20200616090434.1201-1-zhiguang.liu@intel.com> <20200616090434.1201-2-zhiguang.liu@intel.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 16 Jun 2020 16:45:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200616090434.1201-2-zhiguang.liu@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 06/16/20 11:04, Zhiguang Liu wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2317 > > This patch removes EFI_LOADED_IMAGE_PROTOCOL from DXE database. > The SmmCore only install EFI_LOADED_IMAGE_PROTOCOL to SMM database. > > Exposing LOADED_IMAGE_PROTOCOL may cause SMM information leakage > to non-SMM component. > > Cc: Feng Tian > Cc: Star Zeng > Cc: Michael D Kinney > Cc: Laszlo Ersek > Signed-off-by: Jiewen Yao > Signed-off-by: Zhiguang Liu > --- > MdeModulePkg/Core/PiSmmCore/Dispatcher.c | 104 +++++++++++--------------------------------------------------------------------------------------------- > MdeModulePkg/Core/PiSmmCore/PiSmmCore.c | 34 ---------------------------------- > MdeModulePkg/Core/PiSmmCore/PiSmmCore.h | 4 ++-- > 3 files changed, 13 insertions(+), 129 deletions(-) The CC list on this patch is lacking. Per "BaseTools/Scripts/GetMaintainer.py", you should have included the following CC's: Eric Dong Hao A Wu Jian J Wang Ray Ni adding them now. > diff --git a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > index 76ee9e0b89..2be2866234 100644 > --- a/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > +++ b/MdeModulePkg/Core/PiSmmCore/Dispatcher.c > @@ -316,7 +316,7 @@ SmmLoadImage ( > EFI_FIRMWARE_VOLUME2_PROTOCOL *Fv; > PE_COFF_LOADER_IMAGE_CONTEXT ImageContext; > > - PERF_LOAD_IMAGE_BEGIN (DriverEntry->ImageHandle); > + PERF_LOAD_IMAGE_BEGIN (DriverEntry->SmmImageHandle); > > Buffer = NULL; > Size = 0; > @@ -546,51 +546,14 @@ SmmLoadImage ( > DriverEntry->ImageBuffer = DstBuffer; > DriverEntry->NumberOfPage = PageCount; > > - // > - // Allocate a Loaded Image Protocol in EfiBootServicesData > - // > - Status = gBS->AllocatePool (EfiBootServicesData, sizeof (EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&DriverEntry->LoadedImage); > - if (EFI_ERROR (Status)) { > - if (Buffer != NULL) { > - gBS->FreePool (Buffer); > - } > - SmmFreePages (DstBuffer, PageCount); > - return Status; > - } (Side comment: the error path that's being removed here seems to contain a resource leak. We're past PeCoffLoaderLoadImage(), but we're not calling PeCoffLoaderUnloadImage().) > - > - ZeroMem (DriverEntry->LoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL)); > // > // Fill in the remaining fields of the Loaded Image Protocol instance. > - // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed. > // > - DriverEntry->LoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION; > - DriverEntry->LoadedImage->ParentHandle = gSmmCorePrivate->SmmIplImageHandle; > - DriverEntry->LoadedImage->SystemTable = gST; > - DriverEntry->LoadedImage->DeviceHandle = DeviceHandle; > - > DriverEntry->SmmLoadedImage.Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION; > DriverEntry->SmmLoadedImage.ParentHandle = gSmmCorePrivate->SmmIplImageHandle; > DriverEntry->SmmLoadedImage.SystemTable = gST; > DriverEntry->SmmLoadedImage.DeviceHandle = DeviceHandle; > > - // > - // Make an EfiBootServicesData buffer copy of FilePath > - // > - Status = gBS->AllocatePool (EfiBootServicesData, GetDevicePathSize (FilePath), (VOID **)&DriverEntry->LoadedImage->FilePath); > - if (EFI_ERROR (Status)) { > - if (Buffer != NULL) { > - gBS->FreePool (Buffer); > - } > - SmmFreePages (DstBuffer, PageCount); > - return Status; > - } (Side comment: at this point, we've used to leak even "DriverEntry->LoadedImage".) > - CopyMem (DriverEntry->LoadedImage->FilePath, FilePath, GetDevicePathSize (FilePath)); > - > - DriverEntry->LoadedImage->ImageBase = (VOID *)(UINTN) ImageContext.ImageAddress; > - DriverEntry->LoadedImage->ImageSize = ImageContext.ImageSize; > - DriverEntry->LoadedImage->ImageCodeType = EfiRuntimeServicesCode; > - DriverEntry->LoadedImage->ImageDataType = EfiRuntimeServicesData; > - > // > // Make a buffer copy of FilePath > // > @@ -599,7 +562,6 @@ SmmLoadImage ( > if (Buffer != NULL) { > gBS->FreePool (Buffer); > } > - gBS->FreePool (DriverEntry->LoadedImage->FilePath); > SmmFreePages (DstBuffer, PageCount); > return Status; > } (side comment: DriverEntry->LoadedImage was still leaked) > @@ -610,16 +572,6 @@ SmmLoadImage ( > DriverEntry->SmmLoadedImage.ImageCodeType = EfiRuntimeServicesCode; > DriverEntry->SmmLoadedImage.ImageDataType = EfiRuntimeServicesData; > > - // > - // Create a new image handle in the UEFI handle database for the SMM Driver > - // > - DriverEntry->ImageHandle = NULL; > - Status = gBS->InstallMultipleProtocolInterfaces ( > - &DriverEntry->ImageHandle, > - &gEfiLoadedImageProtocolGuid, DriverEntry->LoadedImage, > - NULL > - ); > - > // > // Create a new image handle in the SMM handle database for the SMM Driver > // > @@ -631,7 +583,7 @@ SmmLoadImage ( > &DriverEntry->SmmLoadedImage > ); > > - PERF_LOAD_IMAGE_END (DriverEntry->ImageHandle); > + PERF_LOAD_IMAGE_END (DriverEntry->SmmImageHandle); > > // > // Print the load address and the PDB file name if it is available > @@ -856,7 +808,7 @@ SmmDispatcher ( > // Untrused to Scheduled it would have already been loaded so we may need to > // skip the LoadImage > // > - if (DriverEntry->ImageHandle == NULL) { > + if (DriverEntry->SmmImageHandle == NULL) { > Status = SmmLoadImage (DriverEntry); > > // > @@ -885,8 +837,8 @@ SmmDispatcher ( > REPORT_STATUS_CODE_WITH_EXTENDED_DATA ( > EFI_PROGRESS_CODE, > EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_BEGIN, > - &DriverEntry->ImageHandle, > - sizeof (DriverEntry->ImageHandle) > + &DriverEntry->SmmImageHandle, > + sizeof (DriverEntry->SmmImageHandle) > ); > > // > @@ -898,9 +850,10 @@ SmmDispatcher ( > // For each SMM driver, pass NULL as ImageHandle > // > RegisterSmramProfileImage (DriverEntry, TRUE); > - PERF_START_IMAGE_BEGIN (DriverEntry->ImageHandle); > - Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint)(DriverEntry->ImageHandle, gST); > - PERF_START_IMAGE_END (DriverEntry->ImageHandle); > + DEBUG ((DEBUG_INFO, "SMM StartImage - 0x%11p\n", DriverEntry->ImageEntryPoint)); (1) This DEBUG message belongs in a separate patch, similarly to commit a1ddad95933e ("MdeModulePkg/PiSmmCore: log SMM image start failure", 2020-03-04). > + PERF_START_IMAGE_BEGIN (DriverEntry->SmmImageHandle); > + Status = ((EFI_IMAGE_ENTRY_POINT)(UINTN)DriverEntry->ImageEntryPoint)(DriverEntry->SmmImageHandle, gST); > + PERF_START_IMAGE_END (DriverEntry->SmmImageHandle); > if (EFI_ERROR(Status)){ > DEBUG (( > DEBUG_ERROR, > @@ -910,20 +863,6 @@ SmmDispatcher ( > )); > UnregisterSmramProfileImage (DriverEntry, TRUE); > SmmFreePages(DriverEntry->ImageBuffer, DriverEntry->NumberOfPage); > - // > - // Uninstall LoadedImage > - // > - Status = gBS->UninstallProtocolInterface ( > - DriverEntry->ImageHandle, > - &gEfiLoadedImageProtocolGuid, > - DriverEntry->LoadedImage > - ); > - if (!EFI_ERROR (Status)) { > - if (DriverEntry->LoadedImage->FilePath != NULL) { > - gBS->FreePool (DriverEntry->LoadedImage->FilePath); > - } > - gBS->FreePool (DriverEntry->LoadedImage); > - } > Status = SmmUninstallProtocolInterface ( > DriverEntry->SmmImageHandle, > &gEfiLoadedImageProtocolGuid, > @@ -939,8 +878,8 @@ SmmDispatcher ( > REPORT_STATUS_CODE_WITH_EXTENDED_DATA ( > EFI_PROGRESS_CODE, > EFI_SOFTWARE_SMM_DRIVER | EFI_SW_PC_INIT_END, > - &DriverEntry->ImageHandle, > - sizeof (DriverEntry->ImageHandle) > + &DriverEntry->SmmImageHandle, > + sizeof (DriverEntry->SmmImageHandle) > ); > > if (!PreviousSmmEntryPointRegistered && gSmmCorePrivate->SmmEntryPointRegistered) { > @@ -1344,27 +1283,6 @@ SmmDriverDispatchHandler ( > // > // If this is the SMM core fill in it's DevicePath & DeviceHandle > // > - if (mSmmCoreLoadedImage->FilePath == NULL) { > - // > - // Maybe one special FV contains only one SMM_CORE module, so its device path must > - // be initialized completely. > - // > - EfiInitializeFwVolDevicepathNode (&mFvDevicePath.File, &NameGuid); > - SetDevicePathEndNode (&mFvDevicePath.End); > - > - // > - // Make an EfiBootServicesData buffer copy of FilePath > - // > - Status = gBS->AllocatePool ( > - EfiBootServicesData, > - GetDevicePathSize ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath), > - (VOID **)&mSmmCoreLoadedImage->FilePath > - ); > - ASSERT_EFI_ERROR (Status); > - CopyMem (mSmmCoreLoadedImage->FilePath, &mFvDevicePath, GetDevicePathSize ((EFI_DEVICE_PATH_PROTOCOL *)&mFvDevicePath)); > - > - mSmmCoreLoadedImage->DeviceHandle = FvHandle; > - } > if (mSmmCoreDriverEntry->SmmLoadedImage.FilePath == NULL) { > // > // Maybe one special FV contains only one SMM_CORE module, so its device path must > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > index cfa9922cbd..c1b18b047e 100644 > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.c > @@ -104,8 +104,6 @@ EFI_SMRAM_DESCRIPTOR *mFullSmramRanges; > > EFI_SMM_DRIVER_ENTRY *mSmmCoreDriverEntry; > > -EFI_LOADED_IMAGE_PROTOCOL *mSmmCoreLoadedImage; > - > /** > Place holder function until all the SMM System Table Service are available. > > @@ -756,38 +754,6 @@ SmmCoreInstallLoadedImage ( > ) > { > EFI_STATUS Status; > - EFI_HANDLE Handle; > - > - // > - // Allocate a Loaded Image Protocol in EfiBootServicesData > - // > - Status = gBS->AllocatePool (EfiBootServicesData, sizeof(EFI_LOADED_IMAGE_PROTOCOL), (VOID **)&mSmmCoreLoadedImage); > - ASSERT_EFI_ERROR (Status); > - > - ZeroMem (mSmmCoreLoadedImage, sizeof (EFI_LOADED_IMAGE_PROTOCOL)); > - // > - // Fill in the remaining fields of the Loaded Image Protocol instance. > - // Note: ImageBase is an SMRAM address that can not be accessed outside of SMRAM if SMRAM window is closed. > - // > - mSmmCoreLoadedImage->Revision = EFI_LOADED_IMAGE_PROTOCOL_REVISION; > - mSmmCoreLoadedImage->ParentHandle = gSmmCorePrivate->SmmIplImageHandle; > - mSmmCoreLoadedImage->SystemTable = gST; > - > - mSmmCoreLoadedImage->ImageBase = (VOID *)(UINTN)gSmmCorePrivate->PiSmmCoreImageBase; > - mSmmCoreLoadedImage->ImageSize = gSmmCorePrivate->PiSmmCoreImageSize; > - mSmmCoreLoadedImage->ImageCodeType = EfiRuntimeServicesCode; > - mSmmCoreLoadedImage->ImageDataType = EfiRuntimeServicesData; > - > - // > - // Create a new image handle in the UEFI handle database for the SMM Driver > - // > - Handle = NULL; > - Status = gBS->InstallMultipleProtocolInterfaces ( > - &Handle, > - &gEfiLoadedImageProtocolGuid, mSmmCoreLoadedImage, > - NULL > - ); > - ASSERT_EFI_ERROR (Status); > > // > // Allocate a Loaded Image Protocol in SMM > diff --git a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > index 50a7fc0000..3bbd1e1603 100644 > --- a/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > +++ b/MdeModulePkg/Core/PiSmmCore/PiSmmCore.h > @@ -122,8 +122,8 @@ typedef struct { > BOOLEAN Initialized; > BOOLEAN DepexProtocolError; > > - EFI_HANDLE ImageHandle; > - EFI_LOADED_IMAGE_PROTOCOL *LoadedImage; > + EFI_HANDLE ImageHandle_Reserved1; > + EFI_LOADED_IMAGE_PROTOCOL *LoadedImage_Reserved2; (2) Why are we not removing these fields? (And even if we wanted to keep them, the new names do not conform to the coding style.) > // > // Image EntryPoint in SMRAM > // > Otherwise the patch looks good to me, but I've only done a superficial check. Thanks Laszlo