Thanks Sean.
Acked-by: Jiewen Yao <jiewen....@intel.com>
> -----Original Message-----
> From: Sean Brogan <spbro...@outlook.com>
> Sent: Saturday, January 28, 2023 10:37 AM
> To: Jan Bobek <jbo...@nvidia.com>
> Cc: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>; Sean Brogan
> <sean.bro...@microsoft.com>; Laszlo Ersek <ler...@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v1 0/4] Don't require self-signed PK in setup
> mode
>
> Did i mention how much i hate reviewing code over email? :)
>
> Even after looking at it a few times I missed that change.
>
> I think this change looks fine. Personally, i think this code has
> terrible readability and high complexity and with high impact. It would
> be great to at least get unit tests if not a full refactor of the
> library. I also think the edk2 idea of "custom mode" should be
> removed. But that feedback isn't for this patch and I don't think this
> patch should be held up just because the surrounding code is less than
> ideal.
>
> For patch 1 and 4.
>
> Reviewed-by: Sean Brogan <sean.bro...@microsoft.com>
>
> Thanks
>
> Sean
>
>
>
>
> On 1/27/2023 2:05 PM, Jan Bobek wrote:
> >> I read your replacement a little different.
> >>
> >> - if ((InCustomMode () && UserPhysicalPresent ()) || ((mPlatformMode ==
> SETUP_MODE) && !IsPk)) {
> >>
> >> with
> >>
> >> + if ( (InCustomMode () && UserPhysicalPresent ())
> >> + || ( (mPlatformMode == SETUP_MODE)
> >> + && !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)))
> >> + {
> >>
> >>
> >> In the upper part you replaced it says !IsPk. What am i missing?
> >>
> >>
> >> If a payload was in this function targeting a KEK change with no PK
> >> installed it would go in the original IF condition. In the new code
> >> it would because device is not in custom mode and the payload is not
> >> targeted PK.
> > Is it possible that you're missing the negation (i.e. exclamation mark)
> > in front of the parethesis? The old code was
> >
> > !IsPk
> >
> > The new code is
> >
> > !(FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk)
> >
> > If IsPk is FALSE, both of these evaluate to TRUE no matter what the PCD
> > is.
> >
> > -Jan
> >
> >> On 1/25/2023 1:38 PM, Jan Bobek wrote:
> >>> Hi Sean,
> >>>
> >>>> From looking over the patch 1/4 email i have a concern.
> >>>>
> >>>> In AuthService.c on the conditional change you made. Aren't we losing
> >>>> a case where we are evaluating a nonPK payload signed by the PK?
> >>>> Given the system is in SetupMode that means there is no PK but is this
> >>>> the conditional path that is used when installing Secure boot keys in
> >>>> reverse (DBX,DX,KEK,PK) order?
> >>> Thanks for sharing your concern! They way I think about the change is
> >>> that I've replaced the expression
> >>>
> >>> IsPk
> >>>
> >>> with
> >>>
> >>> FeaturePcdGet (PcdRequireSelfSignedPk) && IsPk
> >>>
> >>> and nothing else. When you look at it this way, it's fairly easy to
> >>> break down how it affects the logic:
> >>>
> >>> 1. Assume PcdRequireSelfSignedPk is TRUE. In this case, the two
> >>> expressions are exactly the same and no change in behavior occurs,
> >>> just as we want.
> >>>
> >>> 2. Assume IsPk is FALSE. In this case, both expressions evaluate to
> >>> FALSE, no matter what the PCD is configured to. That's also good,
> >>> because we don't want to change how non-PK payloads are handled.
> >>>
> >>> 3. In fact, the only change in behavior that occurs is when
> >>> PcdRequireSelfSignedPk is FALSE and IsPk is TRUE; here the former
> >>> expression would be TRUE, whereas the latter is FALSE. That's exactly
> >>> what we want: we wish to enter the first block of the if-else branch
> >>> (which changes the variable similarly to when we're in custom mode)
> >>> rather than falling through to the third block (which checks the
> >>> self-signature).
> >>>
> >>> To directly answer your question, I don't think the behavior changes at
> >>> all when processing non-PK payloads, by virtue of IsPk being FALSE and
> >>> what I said in point (2.) above.
> >>>
> >>> Additionally, yes, the first block of the if-else branch is exactly the
> >>> path that's taken when enrolling KEK/DB/DBX in Setup mode, and one that
> >>> has always been available even without Custom mode. It used to be that
> >>> you couldn't use this path to enroll PK without Custom mode (precisely
> >>> because of !IsPk in the condition), but I'm hoping to enable this path
> >>> with my patch.
> >>>
> >>> Let me know if I haven't answered or misunderstood your question.
> >>>
> >>>> Is there testing you have done? This code should be pretty easy to do
> >>>> host based unit testing on. Any chance you have authored that to
> >>>> confirm use cases are not unexpectedly impacted? Example of host
> >>>> based unit test of library is here:
> >>>> edk2/SecurityPkg/Library/SecureBootVariableLib/UnitTest at master ·
> >>>> tianocore/edk2
> >>>>
> (github.com)<https://nam11.safelinks.protection.outlook.com/?url=https%3A%
> 2F%2Fgithub.com%2Ftianocore%2Fedk2%2Ftree%2Fmaster%2FSecurityPkg%2F
> Library%2FSecureBootVariableLib%2FUnitTest&data=05%7C01%7Cjbobek%40n
> vidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7d
> b39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&sdata=djptmpLUwESbPeqFwFUz5sVbS1MBnD%2FP
> ua9H2y1nxFE%3D&reserved=0>
> >>> I've done an ad-hoc test by commenting out the switch to/from Custom
> >>> mode in EnrollFromDefaultKeysApp from SecurityPkg, booting in Setup
> mode
> >>> and using the modified app to enroll the keys. You could do a similar
> >>> test from the OS, but in my case this was more straightforward.
> >>>
> >>> I am aware of the host-based unit testing library in edk2, and I agree
> >>> that it would be great to have the code in AuthVariableLib tested for
> >>> all the different cases. However, I don't have any such tests right now,
> >>> and while I'm willing to potentially look into writing some, I'd have to
> >>> do it more or less on the side, meaning it could take a while.
> >>>
> >>> Best,
> >>> -Jan
> >>>
> >>>> On 1/22/2023 10:13 PM, Yao, Jiewen wrote:
> >>>>
> >>>> Hi Sean
> >>>> I would like to hear your feedback, since it is a little different from
> >>>> the
> original MSFT patch.
> >>>>
> >>>> Would you please take a look?
> >>>>
> >>>> Thank you
> >>>> Yao, Jiewen
> >>>>
> >>>>
> >>>>
> >>>> -----Original Message-----
> >>>> From: Jan Bobek <jbo...@nvidia.com><mailto:jbo...@nvidia.com>
> >>>> Sent: Saturday, January 21, 2023 6:59 AM
> >>>> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> >>>> Cc: Jan Bobek <jbo...@nvidia.com><mailto:jbo...@nvidia.com>; Laszlo
> Ersek <ler...@redhat.com><mailto:ler...@redhat.com>; Yao,
> >>>> Jiewen <jiewen....@intel.com><mailto:jiewen....@intel.com>
> >>>> Subject: [PATCH v1 0/4] Don't require self-signed PK in setup mode
> >>>>
> >>>> Hi all,
> >>>>
> >>>> I'm sending out v1 of my patch series that addresses a UEFI spec
> >>>> non-compliance when enrolling PK in setup mode. Additional info can be
> >>>> found in bugzilla [1]; the changes are split into 4 patches as
> >>>> suggested by Laszlo Ersek in comment #4.
> >>>>
> >>>> I've based my work on the patch by Matthew Carlson; I've credited him
> >>>> with co-authorship of the first patch even though in the end I decided
> >>>> to do the implementation a bit differently.
> >>>>
> >>>> Comments & reviews welcome!
> >>>>
> >>>> Cheers,
> >>>> -Jan
> >>>>
> >>>> References:
> >>>> 1.
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> tianocore.org%2Fshow_bug.cgi%3Fid%3D2506&data=05%7C01%7Cjbobek%40n
> vidia.com%7Cf2eff15ce44945cc0b4e08db00ad6625%7C43083d15727340c1b7d
> b39efd9ccc17a%7C0%7C0%7C638104516992386171%7CUnknown%7CTWFpbG
> Zsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&sdata=PXam5BVxMmUwgRdGsq1RcnwC8yb8nmO
> zcLz3oKica7s%3D&reserved=0
> >>>>
> >>>> Jan Bobek (4):
> >>>> SecurityPkg: limit verification of enrolled PK in setup mode
> >>>> OvmfPkg: require self-signed PK when secure boot is enabled
> >>>> ArmVirtPkg: require self-signed PK when secure boot is enabled
> >>>> SecurityPkg: don't require PK to be self-signed by default
> >>>>
> >>>> SecurityPkg/SecurityPkg.dec | 7 +++++++
> >>>> ArmVirtPkg/ArmVirtCloudHv.dsc | 4 ++++
> >>>> ArmVirtPkg/ArmVirtQemu.dsc | 4 ++++
> >>>> ArmVirtPkg/ArmVirtQemuKernel.dsc | 4 ++++
> >>>> OvmfPkg/Bhyve/BhyveX64.dsc | 3 +++
> >>>> OvmfPkg/CloudHv/CloudHvX64.dsc | 3 +++
> >>>> OvmfPkg/IntelTdx/IntelTdxX64.dsc | 3 +++
> >>>> OvmfPkg/Microvm/MicrovmX64.dsc | 3 +++
> >>>> OvmfPkg/OvmfPkgIa32.dsc | 3 +++
> >>>> OvmfPkg/OvmfPkgIa32X64.dsc | 3 +++
> >>>> OvmfPkg/OvmfPkgX64.dsc | 3 +++
> >>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf | 3 +++
> >>>> SecurityPkg/Library/AuthVariableLib/AuthService.c | 9 +++++++--
> >>>> 13 files changed, 50 insertions(+), 2 deletions(-)
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99511): https://edk2.groups.io/g/devel/message/99511
Mute This Topic: https://groups.io/mt/96412382/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
