From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web11.7713.1580488345499568260 for ; Fri, 31 Jan 2020 08:32:26 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: michael.d.kinney@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jan 2020 08:31:36 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,386,1574150400"; d="scan'208";a="218656687" Received: from orsmsx110.amr.corp.intel.com ([10.22.240.8]) by orsmga007.jf.intel.com with ESMTP; 31 Jan 2020 08:31:36 -0800 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.57]) by ORSMSX110.amr.corp.intel.com ([169.254.10.111]) with mapi id 14.03.0439.000; Fri, 31 Jan 2020 08:31:36 -0800 From: "Michael D Kinney" To: Laszlo Ersek , "devel@edk2.groups.io" , "Kinney, Michael D" CC: "Zhang, Chao B" , "Wang, Jian J" , "Yao, Jiewen" Subject: Re: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Thread-Topic: [edk2-devel] [PATCH 00/11] SecurityPkg/DxeImageVerificationHandler: fix retval for "deny" policy Thread-Index: AQHVzKAr/+ElsmeVKUm+2G2+2jSfcqgEKWCQgADf9oCAAAPyYA== Date: Fri, 31 Jan 2020 16:31:35 +0000 Message-ID: References: <20200116190705.18816-1-lersek@redhat.com> <45017d12-10e1-8a9b-2997-c8fa42fc1049@redhat.com> In-Reply-To: <45017d12-10e1-8a9b-2997-c8fa42fc1049@redhat.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.22.254.138] MIME-Version: 1.0 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I think a new BZ is a good idea. I am sure there is more=20 history here and more discussion required on this invalid policy PCD setting case. I would also like to see a DEBUG() message or even better a REPORT_STATUS_CODE() for an invalid policy PCD setting and I would like platform policy to decide if the platform should deadloop or continue with EFI_ACCESS_DENIED. By putting the deadloop in this function, it takes away the option for the platform to make that decision. I also find ASSERT(FALSE) harder to triage. I prefer the debug log to provide some indication of the cause of the assert. Then I can go look up the file/line number for more context. Mike > -----Original Message----- > From: Laszlo Ersek > Sent: Friday, January 31, 2020 12:13 AM > To: Kinney, Michael D ; > devel@edk2.groups.io > Cc: Zhang, Chao B ; Wang, Jian > J ; Yao, Jiewen > > Subject: Re: [edk2-devel] [PATCH 00/11] > SecurityPkg/DxeImageVerificationHandler: fix retval for > "deny" policy >=20 > On 01/31/20 03:59, Kinney, Michael D wrote: > > Hi Laszlo, > > > > I have reviewed this patch series. All the patches > look good. The > > detailed description of each change made it easy to > review. > > > > Series Reviewed-by: Michael D Kinney > >=20 > Thank you very much! >=20 > (more below) >=20 > > > > I have one question about that is not directly > related to this logic > > change. > > > > I see this logic that checks for invalid policy > settings and ASSERT()s > > and halts in a deadloop if either of 2 specific > values are used that > > have been removed from the UEFI Spec. > > > > // > > // The policy QUERY_USER_ON_SECURITY_VIOLATION and > ALLOW_EXECUTE_ON_SECURITY_VIOLATION > > // violates the UEFI spec and has been removed. > > // > > ASSERT (Policy !=3D QUERY_USER_ON_SECURITY_VIOLATION > && Policy !=3D ALLOW_EXECUTE_ON_SECURITY_VIOLATION); > > if (Policy =3D=3D QUERY_USER_ON_SECURITY_VIOLATION || > Policy =3D=3D ALLOW_EXECUTE_ON_SECURITY_VIOLATION) { > > CpuDeadLoop (); > > } > > > > There are 3 conditions where the Policy comes from a > PCD. > > > > // > > // Check the image type and get policy setting. > > // > > switch (GetImageType (File)) { > > > > case IMAGE_FROM_FV: > > Policy =3D ALWAYS_EXECUTE; > > break; > > > > case IMAGE_FROM_OPTION_ROM: > > Policy =3D PcdGet32 > (PcdOptionRomImageVerificationPolicy); > > break; > > > > case IMAGE_FROM_REMOVABLE_MEDIA: > > Policy =3D PcdGet32 > (PcdRemovableMediaImageVerificationPolicy); > > break; > > > > case IMAGE_FROM_FIXED_MEDIA: > > Policy =3D PcdGet32 > (PcdFixedMediaImageVerificationPolicy); > > break; > > > > default: > > Policy =3D DENY_EXECUTE_ON_SECURITY_VIOLATION; > > break; > > } > > > > However, there are no checks in this function to > verify that Policy is > > set to one of the allowed values. >=20 > That's right. I seem to recall that I noticed it too, > and I wrote it off > with "you can do damage with a bunch of other PCDs as > well, if you > misconfigure them". >=20 > > This means an invalid PCD value will fall through to > the > > EFI_ACCESS_DENIED case. >=20 > Yes. I find that the safest (silent) fallback for an > undefined (per DEC) > PCD value. >=20 > > Is this the expected behavior for an invalid PCD > setting? If so, then > > why is there a check for the retired values from the > UEFI Spec and a > > deadloop performed. That seems inconsistent and not > a good idea to > > deadloop if we do not have to. Would it be better to > return > > EFI_ACCESS_DENIED for these 2 retired Policy values > as well? >=20 > Hmm, good point. >=20 > I think these scenarios are different, historically. In > one case we have > a plain misconfigured platform. In the other case we > have a platform > that used to have correct configuration (considering > edk2 in itself), > but then for spec conformance reasons, it got > invalidated > *retroactively*. And I guess we wanted to be as loud as > possible about > the second case. There's a difference between "you > didn't do your > homework" and "you did your homework but we've changed > the curriculum > meanwhile". >=20 > So the main question is if we want to remain "silent" > about "plain > misconfig", vs. "loud" about "obsolete config". >=20 > We could unify the handling of both cases ("loudly") > like this, for > example: >=20 > > diff --git > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV > erificationLib.c > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV > erificationLib.c > > index ff79e30ef83e..1d41161abedc 100644 > > --- > a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV > erificationLib.c > > +++ > b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageV > erificationLib.c > > @@ -1617,23 +1617,32 @@ DxeImageVerificationHandler ( > > Policy =3D DENY_EXECUTE_ON_SECURITY_VIOLATION; > > break; > > } > > + > > + switch (Policy) { > > // > > // If policy is always/never execute, return > directly. > > // > > - if (Policy =3D=3D ALWAYS_EXECUTE) { > > + case ALWAYS_EXECUTE: > > return EFI_SUCCESS; > > - } > > - if (Policy =3D=3D NEVER_EXECUTE) { > > + case NEVER_EXECUTE: > > return EFI_ACCESS_DENIED; > > - } > > > > // > > - // The policy QUERY_USER_ON_SECURITY_VIOLATION and > ALLOW_EXECUTE_ON_SECURITY_VIOLATION > > - // violates the UEFI spec and has been removed. > > + // "Defer" and "deny" are valid policies that > require actual verification. > > // > > - ASSERT (Policy !=3D QUERY_USER_ON_SECURITY_VIOLATION > && Policy !=3D ALLOW_EXECUTE_ON_SECURITY_VIOLATION); > > - if (Policy =3D=3D QUERY_USER_ON_SECURITY_VIOLATION || > Policy =3D=3D ALLOW_EXECUTE_ON_SECURITY_VIOLATION) { > > + case DEFER_EXECUTE_ON_SECURITY_VIOLATION: > > + case DENY_EXECUTE_ON_SECURITY_VIOLATION: > > + break; > > + > > + // > > + // QUERY_USER_ON_SECURITY_VIOLATION and > ALLOW_EXECUTE_ON_SECURITY_VIOLATION > > + // violate the UEFI spec; they now indicate > platform misconfig. Hang here if > > + // we find those policies. Handle undefined policy > values the same way. > > + // > > + default: > > + ASSERT (FALSE); > > CpuDeadLoop (); > > + return EFI_ACCESS_DENIED; > > } > > > > GetEfiGlobalVariable2 (EFI_SECURE_BOOT_MODE_NAME, > (VOID**)&SecureBoot, NULL); >=20 > Continuing: >=20 > On 01/31/20 03:59, Kinney, Michael D wrote: > > I am ok if you consider this to be a different issue. >=20 > Yes, TianoCore#2129 is about mistaking DENY for DEFER, > while this other > topic is "complain loudly, rather than silently, about > invalid PCD > settings". >=20 > So let me push this series as-is for TianoCore#2129, > with your R-b > applied. Then I will submit the above patch for > separate review -- I'll > put something like "hang on undefined image > verification policy PCDs" in > the subject. >=20 > Would you like me to file a separate BZ too, for that > patch? >=20 > Thanks! > Laszlo