Laszlo,
I think a new BZ is a good idea. I am sure there is more
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 <ler...@redhat.com>
> Sent: Friday, January 31, 2020 12:13 AM
> To: Kinney, Michael D <michael.d.kin...@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zh...@intel.com>; Wang, Jian
> J <jian.j.w...@intel.com>; Yao, Jiewen
> <jiewen....@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
>
> 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
> <michael.d.kin...@intel.com>
>
> Thank you very much!
>
> (more below)
>
> >
> > 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 != QUERY_USER_ON_SECURITY_VIOLATION
> && Policy != ALLOW_EXECUTE_ON_SECURITY_VIOLATION);
> > if (Policy == QUERY_USER_ON_SECURITY_VIOLATION ||
> Policy == 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 = ALWAYS_EXECUTE;
> > break;
> >
> > case IMAGE_FROM_OPTION_ROM:
> > Policy = PcdGet32
> (PcdOptionRomImageVerificationPolicy);
> > break;
> >
> > case IMAGE_FROM_REMOVABLE_MEDIA:
> > Policy = PcdGet32
> (PcdRemovableMediaImageVerificationPolicy);
> > break;
> >
> > case IMAGE_FROM_FIXED_MEDIA:
> > Policy = PcdGet32
> (PcdFixedMediaImageVerificationPolicy);
> > break;
> >
> > default:
> > Policy = 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.
>
> 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".
>
> > This means an invalid PCD value will fall through to
> the
> > EFI_ACCESS_DENIED case.
>
> Yes. I find that the safest (silent) fallback for an
> undefined (per DEC)
> PCD value.
>
> > 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?
>
> Hmm, good point.
>
> 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".
>
> So the main question is if we want to remain "silent"
> about "plain
> misconfig", vs. "loud" about "obsolete config".
>
> We could unify the handling of both cases ("loudly")
> like this, for
> example:
>
> > 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 = DENY_EXECUTE_ON_SECURITY_VIOLATION;
> > break;
> > }
> > +
> > + switch (Policy) {
> > //
> > // If policy is always/never execute, return
> directly.
> > //
> > - if (Policy == ALWAYS_EXECUTE) {
> > + case ALWAYS_EXECUTE:
> > return EFI_SUCCESS;
> > - }
> > - if (Policy == 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 != QUERY_USER_ON_SECURITY_VIOLATION
> && Policy != ALLOW_EXECUTE_ON_SECURITY_VIOLATION);
> > - if (Policy == QUERY_USER_ON_SECURITY_VIOLATION ||
> Policy == 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);
>
> Continuing:
>
> On 01/31/20 03:59, Kinney, Michael D wrote:
> > I am ok if you consider this to be a different issue.
>
> Yes, TianoCore#2129 is about mistaking DENY for DEFER,
> while this other
> topic is "complain loudly, rather than silently, about
> invalid PCD
> settings".
>
> 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.
>
> Would you like me to file a separate BZ too, for that
> patch?
>
> Thanks!
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53606): https://edk2.groups.io/g/devel/message/53606
Mute This Topic: https://groups.io/mt/69752218/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
