From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Wed, 17 Apr 2019 06:09:53 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E521F81F25; Wed, 17 Apr 2019 13:09:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-123-180.rdu2.redhat.com [10.10.123.180]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2666E1001DCC; Wed, 17 Apr 2019 13:09:50 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2] MdeModulePkg/UefiBootManagerLib: fix crash on uninitialized ExitData To: devel@edk2.groups.io, ard.biesheuvel@linaro.org Cc: jian.j.wang@intel.com, hao.a.wu@intel.com, ray.ni@intel.com, glin@suse.com References: <20190417064047.4676-1-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <735e04d0-08f2-ca4e-1074-9a6bd63406a2@redhat.com> Date: Wed, 17 Apr 2019 15:09:50 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190417064047.4676-1-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Wed, 17 Apr 2019 13:09:53 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 04/17/19 08:40, Ard Biesheuvel wrote: > As reported by Gary, the recent LoadImage/StartImage changes to > accommodate dispatching PE/COFF images built for foreign architectures > may result in a crash when loading an IA32 option ROM into a X64 VM > running OVMF: > > Loading driver at 0x0007E537000 EntryPoint=0x0007E53C06D 8086100e.efi > InstallProtocolInterface: BC62157E-3E33-4FEC-9920-2D3B36D750DF 7F003B98 > ProtectUefiImageCommon - 0x7F002BC0 > - 0x000000007E537000 - 0x000000000009F900 > Image type IA32 can't be started on X64 UEFI system. > ASSERT MdeModulePkg/Core/Dxe/Mem/Pool.c(698): Head->Signature == ((('p') | > ('h' << 8)) | ((('d') | ('0' << 8)) << 16)) || Head->Signature > == ((('p') | ('h' << 8)) | ((('d') | ('1' << 8)) << 16)) > > This turns out to be caused by the deferred image loading code in BDS, > which doesn't check the result code of gBS->StartImage(), and ends up > trying to free an uninitialized pointer. StartImage() can return an error status for one of two reasons: - StartImage() itself fails, or - StartImage() actually transfers control to the image, but then the image exits with an error status. In the latter case, we have two further branches: - the image produces the error status by returning from its entry point function, or - the image calls gBS->Exit(). In the last case, it is possible that the image exits with an error *and* returns some ExitData that needs freeing. (In fact gBS->Exit() can produce ExitData only if it returns an error.) Therefore, a failure status returned by gBS->StartImage() must not prevent an attempt to free a non-NULL ExitData. My point being... the patch is correct, IMO, but the above paragraph of the commit message has not been updated from v1, and it still suggests (to me anyway) that things could be improved by adding a check on "Status". That's not the case, IMO. So here's what I suggest: before pushing the patch, please simply remove the fragment "doesn't check the result code of gBS->StartImage(), and" With that: Reviewed-by: Laszlo Ersek Thanks, Laszlo > > Given that ExitData is never actually used, let's just get rid of it > entirely. While we're at it, drop the pointless assignment of Status > as well. > > Tested-by: Gary Lin > Signed-off-by: Ard Biesheuvel > --- > v2: drop ExitData entirely instead of initializing it to NULL > > MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c > index fc8775dfa419..6b8fb4d92461 100644 > --- a/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c > +++ b/MdeModulePkg/Library/UefiBootManagerLib/BmMisc.c > @@ -439,8 +439,6 @@ EfiBootManagerDispatchDeferredImages ( > UINTN ImageSize; > BOOLEAN BootOption; > EFI_HANDLE ImageHandle; > - UINTN ExitDataSize; > - CHAR16 *ExitData; > UINTN ImageCount; > UINTN LoadCount; > > @@ -502,10 +500,7 @@ EfiBootManagerDispatchDeferredImages ( > // a 5 Minute period > // > gBS->SetWatchdogTimer (5 * 60, 0x0000, 0x00, NULL); > - Status = gBS->StartImage (ImageHandle, &ExitDataSize, &ExitData); > - if (ExitData != NULL) { > - FreePool (ExitData); > - } > + gBS->StartImage (ImageHandle, NULL, NULL); > > // > // Clear the Watchdog Timer after the image returns. >