From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: dandan.bi@intel.com) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by groups.io with SMTP; Tue, 03 Sep 2019 19:07:34 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Sep 2019 19:07:34 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,465,1559545200"; d="scan'208";a="266492556" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga001.jf.intel.com with ESMTP; 03 Sep 2019 19:07:33 -0700 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Sep 2019 19:07:33 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 3 Sep 2019 19:07:33 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.32]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.140]) with mapi id 14.03.0439.000; Wed, 4 Sep 2019 10:07:31 +0800 From: "Dandan Bi" To: Laszlo Ersek , edk2-devel-groups-io CC: Ard Biesheuvel , Leif Lindholm Subject: Re: [PATCH] ArmVirtPkg/PlatformBootManagerLib: unload image on EFI_SECURITY_VIOLATION Thread-Topic: [PATCH] ArmVirtPkg/PlatformBootManagerLib: unload image on EFI_SECURITY_VIOLATION Thread-Index: AQHVYnX4fr6zmz6R70mRlJK2Ads79Kcaxdgw Date: Wed, 4 Sep 2019 02:07:30 +0000 Message-ID: <3C0D5C461C9E904E8F62152F6274C0BB40C56C6F@SHSMSX104.ccr.corp.intel.com> References: <20190903163801.28652-1-lersek@redhat.com> In-Reply-To: <20190903163801.28652-1-lersek@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: dandan.bi@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Dandan Bi Thanks, Dandan > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, September 4, 2019 12:38 AM > To: edk2-devel-groups-io > Cc: Ard Biesheuvel ; Bi, Dandan > ; Leif Lindholm > Subject: [PATCH] ArmVirtPkg/PlatformBootManagerLib: unload image on > EFI_SECURITY_VIOLATION >=20 > The LoadImage() boot service is a bit unusual in that it allocates resour= ces in a > particular failure case; namely, it produces a valid "ImageHandle" when i= t > returns EFI_SECURITY_VIOLATION. This is supposed to happen e.g. when > Secure Boot verification fails for the image, but the platform policy for= the > particular image origin (such as "fixed media" or "removable media") is > DEFER_EXECUTE_ON_SECURITY_VIOLATION. The return code allows > platform logic to selectively override the verification failure, and laun= ch the > image nonetheless. >=20 > ArmVirtPkg/PlatformBootManagerLib does not override > EFI_SECURITY_VIOLATION for the kernel image loaded from fw_cfg -- any > LoadImage() error is considered fatal. When we simply treat > EFI_SECURITY_VIOLATION like any other LoadImage() error, we leak the > resources associated with "KernelImageHandle". From a resource usage > perspective, EFI_SECURITY_VIOLATION must be considered "success", and > rolled back. >=20 > Implement this rollback, without breaking the proper "nesting" of error > handling jumps and labels. >=20 > Cc: Ard Biesheuvel > Cc: Dandan Bi > Cc: Leif Lindholm > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1992 > Fixes: 23d04b58e27b382bbd3f9b16ba9adb1cb203dad5 > Signed-off-by: Laszlo Ersek > --- >=20 > Notes: > Repo: https://github.com/lersek/edk2.git > Branch: ldimg_armvirt_bz1992 >=20 > ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) >=20 > diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > index 3cc83e3b7b95..d3851fd75fa5 100644 > --- a/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/QemuKernel.c > @@ -968,53 +968,60 @@ TryRunningQemuKernel ( >=20 > // > // Create a device path for the kernel image to be loaded from that wi= ll call > // back into our file system. > // > KernelDevicePath =3D FileDevicePath (FileSystemHandle, KernelBlob->Nam= e); > if (KernelDevicePath =3D=3D NULL) { > DEBUG ((EFI_D_ERROR, "%a: failed to allocate kernel device path\n", > __FUNCTION__)); > Status =3D EFI_OUT_OF_RESOURCES; > goto UninstallProtocols; > } >=20 > // > // Load the image. This should call back into our file system. > // > Status =3D gBS->LoadImage ( > FALSE, // BootPolicy: exact match required > gImageHandle, // ParentImageHandle > KernelDevicePath, > NULL, // SourceBuffer > 0, // SourceSize > &KernelImageHandle > ); > if (EFI_ERROR (Status)) { > DEBUG ((EFI_D_ERROR, "%a: LoadImage(): %r\n", __FUNCTION__, > Status)); > - goto FreeKernelDevicePath; > + if (Status !=3D EFI_SECURITY_VIOLATION) { > + goto FreeKernelDevicePath; > + } > + // > + // From the resource allocation perspective, EFI_SECURITY_VIOLATION > means > + // "success", so we must roll back the image loading. > + // > + goto UnloadKernelImage; > } >=20 > // > // Construct the kernel command line. > // > Status =3D gBS->OpenProtocol ( > KernelImageHandle, > &gEfiLoadedImageProtocolGuid, > (VOID **)&KernelLoadedImage, > gImageHandle, // AgentHandle > NULL, // ControllerHandle > EFI_OPEN_PROTOCOL_GET_PROTOCOL > ); > ASSERT_EFI_ERROR (Status); >=20 > if (CommandLineBlob->Data =3D=3D NULL) { > KernelLoadedImage->LoadOptionsSize =3D 0; > } else { > // > // Verify NUL-termination of the command line. > // > if (CommandLineBlob->Data[CommandLineBlob->Size - 1] !=3D '\0') { > DEBUG ((EFI_D_ERROR, "%a: kernel command line is not NUL- > terminated\n", > __FUNCTION__)); > Status =3D EFI_PROTOCOL_ERROR; > goto UnloadKernelImage; > -- > 2.19.1.3.g30247aa5d201