Ref:    https://bugzilla.tianocore.org/show_bug.cgi?id=2129
Repo:   https://github.com/lersek/edk2.git
Branch: deny_execute_2129

The DxeImageVerificationHandler() function does not handle the
DENY_EXECUTE_ON_SECURITY_VIOLATION policy correctly. When an image is
rejected, and the platform sets this policy for the corresponding image
source, the function should return EFI_ACCESS_DENIED. Instead, the
function currently returns EFI_SECURITY_VIOLATION. The consequence is
that gBS->LoadImage() will keep the image loaded (in untrusted state),
rather than unloading it immediately. If the platform sets the
DENY_EXECUTE_ON_SECURITY_VIOLATION policy for all image sources, then
the platform may not expect EFI_SECURITY_VIOLATION at all. Then,
rejected images may linger in RAM, in untrusted state, and may be leaked
forever.

This series refactors the DxeImageVerificationHandler() function,
simplifying the control flow. The series also improves the conformance
of the return values to the SECURITY2_FILE_AUTHENTICATION_HANDLER
prototype. The last two patches are actual bugfixes, with the last one
fixing the problem laid out above.

The patches in this posting have been formatted with
"--function-context", for easier review.

Cc: Chao Zhang <chao.b.zhang@intel.com>
Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>

Thanks,
Laszlo

Laszlo Ersek (11):
  SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus"
  SecurityPkg/DxeImageVerificationHandler: remove "else" after
    return/break
  SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status
    internal
  SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash
    status
  SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc
    failure
  SecurityPkg/DxeImageVerificationHandler: remove superfluous Status
    setting
  SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call
  SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable
  SecurityPkg/DxeImageVerificationHandler: fix retval for
    (FileBuffer==NULL)
  SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc
    fail
  SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny"
    policies

 SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c | 118 ++++++++++----------
 1 file changed, 59 insertions(+), 59 deletions(-)

-- 
2.19.1.3.g30247aa5d201


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53314): https://edk2.groups.io/g/devel/message/53314
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.kinney@intel.com>

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.  This means
an invalid PCD value will fall through to the EFI_ACCESS_DENIED
case.  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?

I am ok if you consider this to be a different issue. 

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Thursday, January 16, 2020 11:07 AM
> To: edk2-devel-groups-io <devel@edk2.groups.io>
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
> 
> Ref:
> https://bugzilla.tianocore.org/show_bug.cgi?id=2129
> Repo:   https://github.com/lersek/edk2.git
> Branch: deny_execute_2129
> 
> The DxeImageVerificationHandler() function does not
> handle the
> DENY_EXECUTE_ON_SECURITY_VIOLATION policy correctly.
> When an image is
> rejected, and the platform sets this policy for the
> corresponding image
> source, the function should return EFI_ACCESS_DENIED.
> Instead, the
> function currently returns EFI_SECURITY_VIOLATION. The
> consequence is
> that gBS->LoadImage() will keep the image loaded (in
> untrusted state),
> rather than unloading it immediately. If the platform
> sets the
> DENY_EXECUTE_ON_SECURITY_VIOLATION policy for all image
> sources, then
> the platform may not expect EFI_SECURITY_VIOLATION at
> all. Then,
> rejected images may linger in RAM, in untrusted state,
> and may be leaked
> forever.
> 
> This series refactors the DxeImageVerificationHandler()
> function,
> simplifying the control flow. The series also improves
> the conformance
> of the return values to the
> SECURITY2_FILE_AUTHENTICATION_HANDLER
> prototype. The last two patches are actual bugfixes,
> with the last one
> fixing the problem laid out above.
> 
> The patches in this posting have been formatted with
> "--function-context", for easier review.
> 
> Cc: Chao Zhang <chao.b.zhang@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 
> Thanks,
> Laszlo
> 
> Laszlo Ersek (11):
>   SecurityPkg/DxeImageVerificationHandler: simplify
> "VerifyStatus"
>   SecurityPkg/DxeImageVerificationHandler: remove
> "else" after
>     return/break
>   SecurityPkg/DxeImageVerificationHandler: keep PE/COFF
> info status
>     internal
>   SecurityPkg/DxeImageVerificationHandler: narrow down
> PE/COFF hash
>     status
>   SecurityPkg/DxeImageVerificationHandler: fix retval
> on memalloc
>     failure
>   SecurityPkg/DxeImageVerificationHandler: remove
> superfluous Status
>     setting
>   SecurityPkg/DxeImageVerificationHandler: unnest
> AddImageExeInfo() call
>   SecurityPkg/DxeImageVerificationHandler: eliminate
> "Status" variable
>   SecurityPkg/DxeImageVerificationHandler: fix retval
> for
>     (FileBuffer==NULL)
>   SecurityPkg/DxeImageVerificationHandler: fix imgexec
> info on memalloc
>     fail
>   SecurityPkg/DxeImageVerificationHandler: fix "defer"
> vs. "deny"
>     policies
> 
> 
> SecurityPkg/Library/DxeImageVerificationLib/DxeImageVer
> ificationLib.c | 118 ++++++++++----------
>  1 file changed, 59 insertions(+), 59 deletions(-)
> 
> --
> 2.19.1.3.g30247aa5d201
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53590): https://edk2.groups.io/g/devel/message/53590
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.kinney@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/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> index ff79e30ef83e..1d41161abedc 100644
> --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
> +++ b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.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 (#53596): https://edk2.groups.io/g/devel/message/53596
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Mike,

On 01/31/20 09:12, Laszlo Ersek wrote:

> So let me push this series as-is for TianoCore#2129, with your R-b
> applied.

My pull request (with the "push" label set) seems to have stalled. The
checks have passed (twice -- I closed and reopened the PR once, to
re-trigger mergify), but the branch is not being merged.

https://github.com/tianocore/edk2/pull/324

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53601): https://edk2.groups.io/g/devel/message/53601
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/31/20 10:28, Laszlo Ersek wrote:
> Hi Mike,
> 
> On 01/31/20 09:12, Laszlo Ersek wrote:
> 
>> So let me push this series as-is for TianoCore#2129, with your R-b
>> applied.
> 
> My pull request (with the "push" label set) seems to have stalled. The
> checks have passed (twice -- I closed and reopened the PR once, to
> re-trigger mergify), but the branch is not being merged.
> 
> https://github.com/tianocore/edk2/pull/324

Merged now: commit range 83357313dd67..8b0932c19f31.

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53603): https://edk2.groups.io/g/devel/message/53603
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

I suspect the behavior you observed was due to the
PR being opened, which triggers the CI actions
immediately.  Then you set the 'push' label, which 
was not seen when the PR was opened.  This required
a second pass which you did by re-opening.

There are 2 approaches a maintainer can take:

1) Open PR without 'push' label.  Wait for CI checks
   to run and review status.  If all pass, then set 
   the 'push' label and re-open.  This option does
   cause 2 passes through the CI checks for a good
   patch series.

2) Open PR with the 'push' label set.  If all checks
   pass, then it is merged.  If any checks fail, then
   the maintainer can address and do a forced push to 
   their branch to retry.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Friday, January 31, 2020 1:28 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
> 
> Hi Mike,
> 
> On 01/31/20 09:12, Laszlo Ersek wrote:
> 
> > So let me push this series as-is for TianoCore#2129,
> with your R-b
> > applied.
> 
> My pull request (with the "push" label set) seems to
> have stalled. The
> checks have passed (twice -- I closed and reopened the
> PR once, to
> re-trigger mergify), but the branch is not being
> merged.
> 
> https://github.com/tianocore/edk2/pull/324
> 
> Thanks
> Laszlo
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53609): https://edk2.groups.io/g/devel/message/53609
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/31/20 17:52, Kinney, Michael D wrote:

> 2) Open PR with the 'push' label set.  If all checks
>    pass, then it is merged.  If any checks fail, then
>    the maintainer can address and do a forced push to 
>    their branch to retry.

Yes, this would be ideal, but I don't know how I can open a PR with the
push label atomically set. Is this available on the WebUI, or just from
the command line?

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53610): https://edk2.groups.io/g/devel/message/53610
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

It can be done with the WebUI. There is a picture on
this page that shows setting the 'push' label before
selecting 'Create Pull Request'

https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

I can also be done with the 'hub' command.  The following
Command shows all the flags for opening a PR using the
hub command.  The '-l' flag allows a label to be set.

  hub help pull-request

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On
> Behalf Of Laszlo Ersek
> Sent: Friday, January 31, 2020 8:59 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: Re: [edk2-devel] [PATCH 00/11]
> SecurityPkg/DxeImageVerificationHandler: fix retval for
> "deny" policy
> 
> On 01/31/20 17:52, Kinney, Michael D wrote:
> 
> > 2) Open PR with the 'push' label set.  If all checks
> >    pass, then it is merged.  If any checks fail, then
> >    the maintainer can address and do a forced push to
> >    their branch to retry.
> 
> Yes, this would be ideal, but I don't know how I can
> open a PR with the
> push label atomically set. Is this available on the
> WebUI, or just from
> the command line?
> 
> Thanks!
> Laszlo
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53614): https://edk2.groups.io/g/devel/message/53614
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/31/20 18:28, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> It can be done with the WebUI. There is a picture on
> this page that shows setting the 'push' label before
> selecting 'Create Pull Request'
> 
> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process

Aha, found it:

https://raw.githubusercontent.com/wiki/tianocore/tianocore.github.io/images/CreateGitHubPullRequest2.png

Many thanks, I'll use it in the future!
Laszlo



>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On
>> Behalf Of Laszlo Ersek
>> Sent: Friday, January 31, 2020 8:59 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>;
>> devel@edk2.groups.io
>> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
>> J <jian.j.wang@intel.com>; Yao, Jiewen
>> <jiewen.yao@intel.com>
>> Subject: Re: [edk2-devel] [PATCH 00/11]
>> SecurityPkg/DxeImageVerificationHandler: fix retval for
>> "deny" policy
>>
>> On 01/31/20 17:52, Kinney, Michael D wrote:
>>
>>> 2) Open PR with the 'push' label set.  If all checks
>>>    pass, then it is merged.  If any checks fail, then
>>>    the maintainer can address and do a forced push to
>>>    their branch to retry.
>>
>> Yes, this would be ideal, but I don't know how I can
>> open a PR with the
>> push label atomically set. Is this available on the
>> WebUI, or just from
>> the command line?
>>
>> Thanks!
>> Laszlo
>>
>>
>> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53622): https://edk2.groups.io/g/devel/message/53622
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Mike,

On 01/31/20 21:19, Laszlo Ersek wrote:
> On 01/31/20 18:28, Kinney, Michael D wrote:
>> Hi Laszlo,
>>
>> It can be done with the WebUI. There is a picture on
>> this page that shows setting the 'push' label before
>> selecting 'Create Pull Request'
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process
> 
> Aha, found it:
> 
> https://raw.githubusercontent.com/wiki/tianocore/tianocore.github.io/images/CreateGitHubPullRequest2.png
> 
> Many thanks, I'll use it in the future!

the label in question is not there for me.

I've just pushed a new topic branch, for the purposes of PR / merging. After running the git-push command locally, I opened the URL in my browser that github.com's remote git daemon printed to my terminal, in response to my git-push command:

> Enumerating objects: 103, done.
> Counting objects: 100% (103/103), done.
> Delta compression using up to 4 threads
> Compressing objects: 100% (76/76), done.
> Writing objects: 100% (76/76), 15.62 KiB | 3.91 MiB/s, done.
> Total 76 (delta 64), reused 0 (delta 0)
> remote: Resolving deltas: 100% (64/64), completed with 24 local objects.
> remote: 
> remote: Create a pull request for 'smram_at_default_smbase_bz_1512_wave_1_v2_pull' on GitHub by visiting:
> remote:      https://github.com/lersek/edk2/pull/new/smram_at_default_smbase_bz_1512_wave_1_v2_pull
> remote: 
> To github.com:lersek/edk2.git
>  * [new branch]                smram_at_default_smbase_bz_1512_wave_1_v2_pull -> smram_at_default_smbase_bz_1512_wave_1_v2_pull

Please see the attachment (screenshot) of what I found under that link. (I even used the browser's "in-page search" function, to locate the word "label"; it was not there.)

Thanks!
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53821): https://edk2.groups.io/g/devel/message/53821
Mute This Topic: https://groups.io/mt/70994494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi Laszlo,

If I follow this link, I see the expected screen with the ability to set a label:

https://github.com/lersek/edk2/pull/new/smram_at_default_smbase_bz_1512_wave_1_v2_pull

If I type in the following URL from your screen shot, I also get the same screen with the ability to set a label:

https://github.com/tianocore/edk2/compare/master...lersek:smram_at_default_smbase_bz_1512_wave_1_v2_pull?expand=1

It also looks like you are logged into GitHub, so that does not appear to be the issue.

I also verified you are a member of EDK II Maintainers, so that does not appear to be the issue.

Can you try both of the links above again?

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, February 5, 2020 5:02 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>
> Subject: setting the push label at once, when opening a
> PR [was: SecurityPkg/DxeImageVerificationHandler: fix
> retval for "deny" policy]
> 
> Hi Mike,
> 
> On 01/31/20 21:19, Laszlo Ersek wrote:
> > On 01/31/20 18:28, Kinney, Michael D wrote:
> >> Hi Laszlo,
> >>
> >> It can be done with the WebUI. There is a picture on
> >> this page that shows setting the 'push' label before
> >> selecting 'Create Pull Request'
> >>
> >>
> https://github.com/tianocore/tianocore.github.io/wiki/E
> DK-II-Development-Process
> >
> > Aha, found it:
> >
> >
> https://raw.githubusercontent.com/wiki/tianocore/tianoc
> ore.github.io/images/CreateGitHubPullRequest2.png
> >
> > Many thanks, I'll use it in the future!
> 
> the label in question is not there for me.
> 
> I've just pushed a new topic branch, for the purposes
> of PR / merging. After running the git-push command
> locally, I opened the URL in my browser that
> github.com's remote git daemon printed to my terminal,
> in response to my git-push command:
> 
> > Enumerating objects: 103, done.
> > Counting objects: 100% (103/103), done.
> > Delta compression using up to 4 threads
> > Compressing objects: 100% (76/76), done.
> > Writing objects: 100% (76/76), 15.62 KiB | 3.91
> MiB/s, done.
> > Total 76 (delta 64), reused 0 (delta 0)
> > remote: Resolving deltas: 100% (64/64), completed
> with 24 local objects.
> > remote:
> > remote: Create a pull request for
> 'smram_at_default_smbase_bz_1512_wave_1_v2_pull' on
> GitHub by visiting:
> > remote:
> https://github.com/lersek/edk2/pull/new/smram_at_defaul
> t_smbase_bz_1512_wave_1_v2_pull
> > remote:
> > To github.com:lersek/edk2.git
> >  * [new branch]
> smram_at_default_smbase_bz_1512_wave_1_v2_pull ->
> smram_at_default_smbase_bz_1512_wave_1_v2_pull
> 
> Please see the attachment (screenshot) of what I found
> under that link. (I even used the browser's "in-page
> search" function, to locate the word "label"; it was
> not there.)
> 
> Thanks!
> Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53831): https://edk2.groups.io/g/devel/message/53831
Mute This Topic: https://groups.io/mt/70994494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 02/05/20 17:16, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> If I follow this link, I see the expected screen with the ability to set a label:
> 
> https://github.com/lersek/edk2/pull/new/smram_at_default_smbase_bz_1512_wave_1_v2_pull
> 
> If I type in the following URL from your screen shot, I also get the same screen with the ability to set a label:
> 
> https://github.com/tianocore/edk2/compare/master...lersek:smram_at_default_smbase_bz_1512_wave_1_v2_pull?expand=1
> 
> It also looks like you are logged into GitHub, so that does not appear to be the issue.
> 
> I also verified you are a member of EDK II Maintainers, so that does not appear to be the issue.
> 
> Can you try both of the links above again?

I've just done that, I even disabled uBlock Origin temporarily. No
change -- I still don't get the labels selection, under either link. And
I'm still logged in.

Perhaps the problem is that I don't have write access to the repo.

https://help.github.com/en/github/managing-your-work-on-github/labeling-issues-and-pull-requests
https://help.github.com/en/github/managing-your-work-on-github/applying-labels-to-issues-and-pull-requests

"In repositories where you have write access, you can assign labels to
issues and pull requests to help organize your projects."

(But then I don't understand why I'm permitted to set the "push" label
on an existent PR...)

Thanks
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53833): https://edk2.groups.io/g/devel/message/53833
Mute This Topic: https://groups.io/mt/70994494/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/31/20 10:28, Laszlo Ersek wrote:
> Hi Mike,
>
> On 01/31/20 09:12, Laszlo Ersek wrote:
>
>> So let me push this series as-is for TianoCore#2129, with your R-b
>> applied.
>
> My pull request (with the "push" label set) seems to have stalled. The
> checks have passed (twice -- I closed and reopened the PR once, to
> re-trigger mergify), but the branch is not being merged.
>
> https://github.com/tianocore/edk2/pull/324

BTW, here are the changes between the posted & reviewed series, and the
pull request:

- I had to replace an EFI_D_INFO macro with DEBUG_INFO, due to
  checkpatch complaints. (The macro is not introduced anew, it is
  touched only by un-indenting.)

- Normal administrativa (picked up R-b tags and Message-Id's, and noted
  Mike substituting for the SecurityPkg reviewers during the CNY
  holidays)

See the git-range-diff output after my sig.

Thanks,
Laszlo

 1:  71155b00b2b7 !  1:  4c8cd26ce423 SecurityPkg/DxeImageVerificationHandler: simplify "VerifyStatus"
    @@ -19,6 +19,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-2-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
 2:  9ad18d2e3adb !  2:  f04114b6d6b2 SecurityPkg/DxeImageVerificationHandler: remove "else" after return/break
    @@ -45,6 +45,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-3-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
 3:  e211153f9a32 !  3:  da0e0dfc67c4 SecurityPkg/DxeImageVerificationHandler: keep PE/COFF info status internal
    @@ -35,6 +35,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-4-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
 4:  3ad36b80defa !  4:  d930abc95422 SecurityPkg/DxeImageVerificationHandler: narrow down PE/COFF hash status
    @@ -26,6 +26,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-5-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
 5:  379ac43e909b !  5:  91b24a413440 SecurityPkg/DxeImageVerificationHandler: fix retval on memalloc failure
    @@ -21,6 +21,11 @@
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Fixes: 570b3d1a7278df29878da87990e8366bd42d0ec5
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-6-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
 6:  c53a99ceb9f2 !  6:  937d1c73965e SecurityPkg/DxeImageVerificationHandler: remove superfluous Status setting
    @@ -13,6 +13,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-7-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
 7:  c259648bbb30 !  7:  be0040ffa6cf SecurityPkg/DxeImageVerificationHandler: unnest AddImageExeInfo() call
    @@ -20,6 +20,12 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-8-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: replace EFI_D_INFO w/ DEBUG_INFO for PatchCheck.py]
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
    @@ -101,7 +107,7 @@
     +  NameStr = ConvertDevicePathToText (File, FALSE, TRUE);
     +  AddImageExeInfo (Action, NameStr, File, SignatureList, SignatureListSize);
     +  if (NameStr != NULL) {
    -+    DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", NameStr));
    ++    DEBUG ((DEBUG_INFO, "The image doesn't pass verification: %s\n", NameStr));
     +    FreePool(NameStr);
        }
     +  Status = EFI_SECURITY_VIOLATION;
 8:  ca43b52bbd96 !  8:  feffd6bfd886 SecurityPkg/DxeImageVerificationHandler: eliminate "Status" variable
    @@ -17,6 +17,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-9-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
    @@ -38,7 +43,7 @@


     @@
    -     DEBUG((EFI_D_INFO, "The image doesn't pass verification: %s\n", NameStr));
    +     DEBUG ((DEBUG_INFO, "The image doesn't pass verification: %s\n", NameStr));
          FreePool(NameStr);
        }
     -  Status = EFI_SECURITY_VIOLATION;
 9:  22edc076c210 !  9:  116742d3de8f SecurityPkg/DxeImageVerificationHandler: fix retval for (FileBuffer==NULL)
    @@ -21,6 +21,11 @@
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Fixes: 570b3d1a7278df29878da87990e8366bd42d0ec5
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-10-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
10:  e0b5e3b25eff ! 10:  b73c1a576b78 SecurityPkg/DxeImageVerificationHandler: fix imgexec info on memalloc fail
    @@ -28,6 +28,11 @@
         Cc: Jiewen Yao <jiewen.yao@intel.com>
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-11-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
11:  60363427926f ! 11:  1493b3ebadca SecurityPkg/DxeImageVerificationHandler: fix "defer" vs. "deny" policies
    @@ -37,6 +37,11 @@
         Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2129
         Fixes: 5db28a6753d307cdfb1cfdeb2f63739a9f959837
         Signed-off-by: Laszlo Ersek <lersek@redhat.com>
    +    Message-Id: <20200116190705.18816-12-lersek@redhat.com>
    +    Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>
    +    [lersek@redhat.com: push with Mike's R-b due to Chinese New Year
    +     Holiday: <https://edk2.groups.io/g/devel/message/53429>; msgid
    +     <d3fbb76dabed4e1987c512c328c82810@intel.com>]

     diff --git a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c b/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c
     --- a/SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.c


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53602): https://edk2.groups.io/g/devel/message/53602
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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 <lersek@redhat.com>
> Sent: Friday, January 31, 2020 12:13 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Zhang, Chao B <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Yao, Jiewen
> <jiewen.yao@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.kinney@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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/31/20 17:31, Kinney, Michael D wrote:
> 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.

OK. I'll abandon the patch, and only open a BZ with this information.
It's best if the SecurityPkg reviewers evaluate it carefully.

Thanks!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53611): https://edk2.groups.io/g/devel/message/53611
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 01/31/20 18:00, Laszlo Ersek wrote:
> On 01/31/20 17:31, Kinney, Michael D wrote:
>> 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.
> 
> OK. I'll abandon the patch, and only open a BZ with this information.
> It's best if the SecurityPkg reviewers evaluate it carefully.

Here's the ticket:

https://bugzilla.tianocore.org/show_bug.cgi?id=2497

Thanks,
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#53613): https://edk2.groups.io/g/devel/message/53613
Mute This Topic: https://groups.io/mt/69752218/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-