[edk2-devel] [RFC] Secure boot default key


Grzegorz Bernacki
 

Hello,

I would like to ask you to review the proposal for new application for
enrolling Secure Boot keys. The application uses content of PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing

Please let me know, if you have an questions, comments or doubts.
thanks,
Greg


Laszlo Ersek
 

On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application for
enrolling Secure Boot keys. The application uses content of PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing
- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.

Thanks,
Laszlo


Laszlo Ersek
 

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application for
enrolling Secure Boot keys. The application uses content of PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing
- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo


Samer El-Haj-Mahmoud
 

Adding SecurityPkg and Arm maintainers

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 16, 2021 8:16 AM
To: rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Samer El-Haj-Mahmoud <Samer.El-
Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul Yang
<Paul.Yang@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application
for enrolling Secure Boot keys. The application uses content of
PKDefault, KEKDefault, dbDefault, dbtDefault and dbxDefault to set
Secure Boot key. Default variables are initialized based on keys
embedded in flash binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/vie
w?usp=sharing
- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at
least it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature
enabled would have to pass a -D flag (boolean) for enabling the
feature in general (enabling the necessary lines in both the DSC and
the FDF file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too, under
SecurityPkg, so that any platform utilizing this feature do so through identical
macro names.

Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Grzegorz Bernacki
 

Hi Laszlo,

Thanks a lot for your review. I will wait some time for more comments
and I will send an updated design.
thanks,
greg

pt., 16 kwi 2021 o 14:16 Laszlo Ersek <lersek@redhat.com> napisał(a):


On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application for
enrolling Secure Boot keys. The application uses content of PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing
- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo


Min Xu <min.m.xu@...>
 

Agree that it is a good idea to provide a mechanism to enroll the Secure boot keys.

But why not add a standalone tool in BaseTools? For example a Python scripts to enroll the Secure Boot keys. I think there are below benefits:
- The usage of the tool can be flexible. For example, the developer,
validation guys can invoke this tool to enroll keys to do the test.
Even the CI can leverage this tool to enroll keys in post build phase.
- Secure Boot keys can be easily updated. Furthermore the tool can do
more checking on the keys, such as the cert format, key strength, etc.
- Keep EDK2 focusing on the key functions. Some other nice-to-have
functions can be implemented by the tools in BaseTools.

-----Original Message-----
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Sent: Saturday, April 17, 2021 3:00 AM
To: Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io;
gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang
<Sunny.Wang@arm.com>; Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Xu, Min M
<min.m.xu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen
<jiewen.yao@intel.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@arm.com>
Subject: RE: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Adding SecurityPkg and Arm maintainers



-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 16, 2021 8:16 AM
To: rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Samer El-Haj-Mahmoud
<Samer.El-
Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul
Yang
<Paul.Yang@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application
for enrolling Secure Boot keys. The application uses content of
PKDefault, KEKDefault, dbDefault, dbtDefault and dbxDefault to set
Secure Boot key. Default variables are initialized based on keys
embedded in flash binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/v
ie
w?usp=sharing
- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be
testable with OVMF, provided the FDF file(s) are modified as needed;
however, I wouldn't like to have that in the FDF file(s) permanently
-- or at least it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well
(for the pathnames). So a user building a platform with this feature
enabled would have to pass a -D flag (boolean) for enabling the
feature in general (enabling the necessary lines in both the DSC and
the FDF file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.


Laszlo Ersek
 

On 04/26/21 10:14, Sunny Wang wrote:
Hi Min Xu,

Yeah, the standalone tool is good and can bring the benefits you mentioned, but I'm still not clear on the standalone tool. Could you give us more information about the standalone tool? Do you mean to have a standalone tool to directly add/modify default secure boot keys in the pre-build Var Store FD image/FV? If so, we thought about that. However, some platforms like RPi4 don't have the pre-build Var Store FD image/FV and may not want to add this to take additional flash space, so I think we can still go with the current proposal first to cover this case and generally cover all the cases. Then, we can separately work on the standalone tool. What do you guys think?

Moreover, there is another thing I'm confused about. We found https://github.com/jyao1/edk2-staging/blob/TDVF/TdvfPkg/scripts/VarEnroll.py that is in the branch owned by you and Jiewen. Is VarEnroll.py the standalone tool you mentioned?
In discussions where I've been present over the last several years, such
a tool has been brought up several times. I always resist the idea.
Here's why:

- Such a host-side program needs to duplicate the functionality of three
firmware drivers, composed. Namely: the low-level flash driver (which
includes flash size / location / structure), the FTW driver, and the
variable driver. It's a double maintenance nightmare. Of course if
someone other than myself is willing to maintain the python tool in
parallel to the edk2 drivers proper, then please be my guest.

- The expectation is usually "let me tweak the varstore at any point,
from the host side, not just for initial enrollment". That means the
tool may face a variable store that is not "clean". For example, power
loss during variable update. The FTW driver deals with this within the
firmware, but the same kind of journaling / rollback / spare area
handling / reclaim etc etc would have to be implemented in the host side
utility. Nah.

Thanks
Laszlo


Best Regards,
Sunny Wang

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Min Xu via groups.io
Sent: Monday, April 26, 2021 10:18 AM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul Yang <Paul.Yang@arm.com>; ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Agree that it is a good idea to provide a mechanism to enroll the Secure boot keys.

But why not add a standalone tool in BaseTools? For example a Python scripts to enroll the Secure Boot keys. I think there are below benefits:
- The usage of the tool can be flexible. For example, the developer,
validation guys can invoke this tool to enroll keys to do the test.
Even the CI can leverage this tool to enroll keys in post build phase.
- Secure Boot keys can be easily updated. Furthermore the tool can do
more checking on the keys, such as the cert format, key strength, etc.
- Keep EDK2 focusing on the key functions. Some other nice-to-have
functions can be implemented by the tools in BaseTools.

-----Original Message-----
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Sent: Saturday, April 17, 2021 3:00 AM
To: Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io;
gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang <Sunny.Wang@arm.com>;
Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Xu, Min
ardb+M
<min.m.xu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao,
Jiewen <jiewen.yao@intel.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@arm.com>
Subject: RE: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Adding SecurityPkg and Arm maintainers



-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 16, 2021 8:16 AM
To: rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Samer El-Haj-Mahmoud
<Samer.El-
Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul
Yang
<Paul.Yang@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new
application for enrolling Secure Boot keys. The application uses
content of PKDefault, KEKDefault, dbDefault, dbtDefault and
dbxDefault to set Secure Boot key. Default variables are
initialized based on keys embedded in flash binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/v
ie
w?usp=sharing
- A new platform DXE driver (which need not even stay resident),
for locating some FFS files and their sections, and for copying
their contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile
in order to avoid unnecessary bloating of the consumed
non-volatile storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG.
A platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be
testable with OVMF, provided the FDF file(s) are modified as
needed; however, I wouldn't like to have that in the FDF file(s)
permanently
-- or at least it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well
(for the pathnames). So a user building a platform with this
feature enabled would have to pass a -D flag (boolean) for
enabling the feature in general (enabling the necessary lines in
both the DSC and the FDF file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files)
too, under SecurityPkg, so that any platform utilizing this feature
do so through identical macro names.

Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient, please notify the sender immediately and do not disclose
the contents to any other person, use it for any purpose, or store or
copy the information in any medium. Thank you.




IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Min Xu <min.m.xu@...>
 

On April 26, 2021 4:15 PM, Sunny Wang wrote:
Hi Min Xu,

Yeah, the standalone tool is good and can bring the benefits you mentioned,
but I'm still not clear on the standalone tool. Could you give us more
information about the standalone tool? Do you mean to have a standalone
tool to directly add/modify default secure boot keys in the pre-build Var
Store FD image/FV? If so, we thought about that. However, some platforms
like RPi4 don't have the pre-build Var Store FD image/FV and may not want
to add this to take additional flash space, so I think we can still go with the
current proposal first to cover this case and generally cover all the cases.
Then, we can separately work on the standalone tool. What do you guys
think?
Yes, a standalone tool I mean is the one that directly add/modify default secure boot keys in the pre-build Var Store FV.
I also want to hear other people's opinions on this RFC.


Moreover, there is another thing I'm confused about. We found
https://github.com/jyao1/edk2-
staging/blob/TDVF/TdvfPkg/scripts/VarEnroll.py that is in the branch owned
by you and Jiewen. Is VarEnroll.py the standalone tool you mentioned?
Yes, VarEnroll.py is the tool we used in TDVF (TDX Virtual Firmware) to enroll secure boot keys.


Best Regards,
Sunny Wang

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Min Xu via
groups.io
Sent: Monday, April 26, 2021 10:18 AM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Laszlo
Ersek <lersek@redhat.com>; rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang
<Sunny.Wang@arm.com>; Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Wang, Jian J
<jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Agree that it is a good idea to provide a mechanism to enroll the Secure boot
keys.

But why not add a standalone tool in BaseTools? For example a Python
scripts to enroll the Secure Boot keys. I think there are below benefits:
- The usage of the tool can be flexible. For example, the developer,
validation guys can invoke this tool to enroll keys to do the test.
Even the CI can leverage this tool to enroll keys in post build phase.
- Secure Boot keys can be easily updated. Furthermore the tool can do
more checking on the keys, such as the cert format, key strength, etc.
- Keep EDK2 focusing on the key functions. Some other nice-to-have
functions can be implemented by the tools in BaseTools.

-----Original Message-----
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Sent: Saturday, April 17, 2021 3:00 AM
To: Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io;
gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang
<Sunny.Wang@arm.com>;
Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Xu, Min
ardb+M
<min.m.xu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao,
Jiewen <jiewen.yao@intel.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@arm.com>
Subject: RE: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Adding SecurityPkg and Arm maintainers



-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 16, 2021 8:16 AM
To: rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Samer El-Haj-Mahmoud
<Samer.El-
Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul
Yang
<Paul.Yang@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new
application for enrolling Secure Boot keys. The application uses
content of PKDefault, KEKDefault, dbDefault, dbtDefault and
dbxDefault to set Secure Boot key. Default variables are
initialized based on keys embedded in flash binary file. Please find
details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/v
ie
w?usp=sharing
- A new platform DXE driver (which need not even stay resident),
for locating some FFS files and their sections, and for copying
their contents into PKDefault and friends, seems like a good / useful
idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile
in order to avoid unnecessary bloating of the consumed
non-volatile storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG.
A platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be
testable with OVMF, provided the FDF file(s) are modified as
needed; however, I wouldn't like to have that in the FDF file(s)
permanently
-- or at least it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well
(for the pathnames). So a user building a platform with this
feature enabled would have to pass a -D flag (boolean) for
enabling the feature in general (enabling the necessary lines in
both the DSC and the FDF file(s)), and then further -D flags for
specifying the pathnames.

You could perhaps introduce DSC and FDF snippets (include files)
too, under SecurityPkg, so that any platform utilizing this feature
do so through identical macro names.

Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient, please notify the sender immediately and do not disclose
the contents to any other person, use it for any purpose, or store or
copy the information in any medium. Thank you.




IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended recipient,
please notify the sender immediately and do not disclose the contents to any
other person, use it for any purpose, or store or copy the information in any
medium. Thank you.


Sunny Wang
 

Hi Min Xu,

Yeah, the standalone tool is good and can bring the benefits you mentioned, but I'm still not clear on the standalone tool. Could you give us more information about the standalone tool? Do you mean to have a standalone tool to directly add/modify default secure boot keys in the pre-build Var Store FD image/FV? If so, we thought about that. However, some platforms like RPi4 don't have the pre-build Var Store FD image/FV and may not want to add this to take additional flash space, so I think we can still go with the current proposal first to cover this case and generally cover all the cases. Then, we can separately work on the standalone tool. What do you guys think?

Moreover, there is another thing I'm confused about. We found https://github.com/jyao1/edk2-staging/blob/TDVF/TdvfPkg/scripts/VarEnroll.py that is in the branch owned by you and Jiewen. Is VarEnroll.py the standalone tool you mentioned?

Best Regards,
Sunny Wang

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Min Xu via groups.io
Sent: Monday, April 26, 2021 10:18 AM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul Yang <Paul.Yang@arm.com>; ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Agree that it is a good idea to provide a mechanism to enroll the Secure boot keys.

But why not add a standalone tool in BaseTools? For example a Python scripts to enroll the Secure Boot keys. I think there are below benefits:
- The usage of the tool can be flexible. For example, the developer,
validation guys can invoke this tool to enroll keys to do the test.
Even the CI can leverage this tool to enroll keys in post build phase.
- Secure Boot keys can be easily updated. Furthermore the tool can do
more checking on the keys, such as the cert format, key strength, etc.
- Keep EDK2 focusing on the key functions. Some other nice-to-have
functions can be implemented by the tools in BaseTools.

-----Original Message-----
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Sent: Saturday, April 17, 2021 3:00 AM
To: Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io;
gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang <Sunny.Wang@arm.com>;
Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Xu, Min
ardb+M
<min.m.xu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao,
Jiewen <jiewen.yao@intel.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@arm.com>
Subject: RE: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Adding SecurityPkg and Arm maintainers



-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 16, 2021 8:16 AM
To: rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Samer El-Haj-Mahmoud
<Samer.El-
Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul
Yang
<Paul.Yang@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new
application for enrolling Secure Boot keys. The application uses
content of PKDefault, KEKDefault, dbDefault, dbtDefault and
dbxDefault to set Secure Boot key. Default variables are
initialized based on keys embedded in flash binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/v
ie
w?usp=sharing
- A new platform DXE driver (which need not even stay resident),
for locating some FFS files and their sections, and for copying
their contents into PKDefault and friends, seems like a good / useful idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile
in order to avoid unnecessary bloating of the consumed
non-volatile storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG.
A platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be
testable with OVMF, provided the FDF file(s) are modified as
needed; however, I wouldn't like to have that in the FDF file(s)
permanently
-- or at least it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well
(for the pathnames). So a user building a platform with this
feature enabled would have to pass a -D flag (boolean) for
enabling the feature in general (enabling the necessary lines in
both the DSC and the FDF file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files)
too, under SecurityPkg, so that any platform utilizing this feature
do so through identical macro names.

Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient, please notify the sender immediately and do not disclose
the contents to any other person, use it for any purpose, or store or
copy the information in any medium. Thank you.




IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


Ethin Probst
 

I agree with Laszlo on this. Duplicating functionality violates DRY
and also could introduce major inconsistencies between how the tool
functions and how the firmware does the same. The only way to
alleviate this problem is for us to (somehow) make the Python tool (or
Rust tool or whatever language we decide to write the tool in)
interface with the C code powering the DXE driver in the
firmware/Security PKG. I know that Python and Rust can do this, via
`ctypes` and `extern "C"` respectively, but that may not be possible
-- we'd need to build the library for both the host platform/operating
system/CPU architecture and the target architecture/UEFI.

On 4/26/21, Min Xu <min.m.xu@intel.com> wrote:
On April 26, 2021 4:15 PM, Sunny Wang wrote:
Hi Min Xu,

Yeah, the standalone tool is good and can bring the benefits you
mentioned,
but I'm still not clear on the standalone tool. Could you give us more
information about the standalone tool? Do you mean to have a standalone
tool to directly add/modify default secure boot keys in the pre-build Var
Store FD image/FV? If so, we thought about that. However, some platforms
like RPi4 don't have the pre-build Var Store FD image/FV and may not want
to add this to take additional flash space, so I think we can still go
with the
current proposal first to cover this case and generally cover all the
cases.
Then, we can separately work on the standalone tool. What do you guys
think?
Yes, a standalone tool I mean is the one that directly add/modify default
secure boot keys in the pre-build Var Store FV.
I also want to hear other people's opinions on this RFC.


Moreover, there is another thing I'm confused about. We found
https://github.com/jyao1/edk2-
staging/blob/TDVF/TdvfPkg/scripts/VarEnroll.py that is in the branch
owned
by you and Jiewen. Is VarEnroll.py the standalone tool you mentioned?
Yes, VarEnroll.py is the tool we used in TDVF (TDX Virtual Firmware) to
enroll secure boot keys.


Best Regards,
Sunny Wang

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Min Xu via
groups.io
Sent: Monday, April 26, 2021 10:18 AM
To: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>; Laszlo
Ersek <lersek@redhat.com>; rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang
<Sunny.Wang@arm.com>; Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Wang, Jian
J
<jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Agree that it is a good idea to provide a mechanism to enroll the Secure
boot
keys.

But why not add a standalone tool in BaseTools? For example a Python
scripts to enroll the Secure Boot keys. I think there are below benefits:
- The usage of the tool can be flexible. For example, the developer,
validation guys can invoke this tool to enroll keys to do the test.
Even the CI can leverage this tool to enroll keys in post build phase.
- Secure Boot keys can be easily updated. Furthermore the tool can do
more checking on the keys, such as the cert format, key strength, etc.
- Keep EDK2 focusing on the key functions. Some other nice-to-have
functions can be implemented by the tools in BaseTools.

-----Original Message-----
From: Samer El-Haj-Mahmoud <Samer.El-Haj-Mahmoud@arm.com>
Sent: Saturday, April 17, 2021 3:00 AM
To: Laszlo Ersek <lersek@redhat.com>; rfc@edk2.groups.io;
gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Sunny Wang
<Sunny.Wang@arm.com>;
Paul Yang <Paul.Yang@arm.com>;
ardb+tianocore@kernel.org; Leif Lindholm <leif@nuviainc.com>; Xu, Min
ardb+M
<min.m.xu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Yao,
Jiewen <jiewen.yao@intel.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
Mahmoud@arm.com>
Subject: RE: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

Adding SecurityPkg and Arm maintainers



-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 16, 2021 8:16 AM
To: rfc@edk2.groups.io; gjb@semihalf.com
Cc: Marcin Wojtas <mw@semihalf.com>; Samer El-Haj-Mahmoud
<Samer.El-
Haj-Mahmoud@arm.com>; Sunny Wang <Sunny.Wang@arm.com>; Paul
Yang
<Paul.Yang@arm.com>
Subject: Re: [edk2-rfc] [edk2-devel] [RFC] Secure boot default key

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new
application for enrolling Secure Boot keys. The application uses
content of PKDefault, KEKDefault, dbDefault, dbtDefault and
dbxDefault to set Secure Boot key. Default variables are
initialized based on keys embedded in flash binary file. Please
find
details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/v
ie
w?usp=sharing
- A new platform DXE driver (which need not even stay resident),
for locating some FFS files and their sections, and for copying
their contents into PKDefault and friends, seems like a good /
useful
idea to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile
in order to avoid unnecessary bloating of the consumed
non-volatile storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG.
A platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be
testable with OVMF, provided the FDF file(s) are modified as
needed; however, I wouldn't like to have that in the FDF file(s)
permanently
-- or at least it should be gated with a new build time feature test
macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well
(for the pathnames). So a user building a platform with this
feature enabled would have to pass a -D flag (boolean) for
enabling the feature in general (enabling the necessary lines in
both the DSC and the FDF file(s)), and then further -D flags for
specifying the pathnames.

You could perhaps introduce DSC and FDF snippets (include files)
too, under SecurityPkg, so that any platform utilizing this feature
do so through identical macro names.

Thanks
Laszlo
IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient, please notify the sender immediately and do not disclose
the contents to any other person, use it for any purpose, or store or
copy the information in any medium. Thank you.




IMPORTANT NOTICE: The contents of this email and any attachments are
confidential and may also be privileged. If you are not the intended
recipient,
please notify the sender immediately and do not disclose the contents to
any
other person, use it for any purpose, or store or copy the information in
any
medium. Thank you.




--
Signed,
Ethin D. Probst


Grzegorz Bernacki
 

Hi,

Thanks a lot for reviewing my previous document. Please see below link to
updated design:
https://drive.google.com/file/d/11yNJJ2x8WXYZRg1nZWrr4C4Hq89Izn1D/view?usp=sharing

Changes:
- remove unnecessary description of default variables
- add macros to specify default keys files
- add fdf include file
- add UI to reset the content of the keys to default values

Please let me know if you have any questions/comments/concerns.
thanks,
greg

pon., 19 kwi 2021 o 11:07 Grzegorz Bernacki <gjb@semihalf.com> napisał(a):

Hi Laszlo,

Thanks a lot for your review. I will wait some time for more comments
and I will send an updated design.
thanks,
greg

pt., 16 kwi 2021 o 14:16 Laszlo Ersek <lersek@redhat.com> napisał(a):

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application for
enrolling Secure Boot keys. The application uses content of PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing

- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea
to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at
least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo


Laszlo Ersek
 

Hi Greg,

On 04/28/21 12:50, Grzegorz Bernacki wrote:
Hi,

Thanks a lot for reviewing my previous document. Please see below link to
updated design:
https://drive.google.com/file/d/11yNJJ2x8WXYZRg1nZWrr4C4Hq89Izn1D/view?usp=sharing

Changes:
- remove unnecessary description of default variables
- add macros to specify default keys files
- add fdf include file
- add UI to reset the content of the keys to default values
the master format for this document seems to be LaTeX (from the "pdf
properties" window in evince), which is fantastic: can you please post a
diff between the v1 and v2 LaTeX sources?

I can only deal with incremental reviews for v2 and later postings. Most
WYSIWYG document editors nowadays support one form of change tracking or
another. Working with a plaintext source format such as LaTeX is always
best, of course. My only request is then, "diffs please".

The rendered format is absolutely welcome in addition to the diffs, of
course.

Thanks
Laszlo



Please let me know if you have any questions/comments/concerns.
thanks,
greg

pon., 19 kwi 2021 o 11:07 Grzegorz Bernacki <gjb@semihalf.com> napisał(a):

Hi Laszlo,

Thanks a lot for your review. I will wait some time for more comments
and I will send an updated design.
thanks,
greg

pt., 16 kwi 2021 o 14:16 Laszlo Ersek <lersek@redhat.com> napisał(a):

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application for
enrolling Secure Boot keys. The application uses content of PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing

- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea
to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at
least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo




Grzegorz Bernacki
 

Hi Laszlo,

Please find diff at following link:
https://drive.google.com/file/d/1ZEzNUjaz9PiQPnQlIfsHYiJ6ZCda_szD/view?usp=sharing

thanks,
greg

śr., 28 kwi 2021 o 18:55 Laszlo Ersek <lersek@redhat.com> napisał(a):

Hi Greg,

On 04/28/21 12:50, Grzegorz Bernacki wrote:
Hi,

Thanks a lot for reviewing my previous document. Please see below link to
updated design:
https://drive.google.com/file/d/11yNJJ2x8WXYZRg1nZWrr4C4Hq89Izn1D/view?usp=sharing

Changes:
- remove unnecessary description of default variables
- add macros to specify default keys files
- add fdf include file
- add UI to reset the content of the keys to default values
the master format for this document seems to be LaTeX (from the "pdf
properties" window in evince), which is fantastic: can you please post a
diff between the v1 and v2 LaTeX sources?

I can only deal with incremental reviews for v2 and later postings. Most
WYSIWYG document editors nowadays support one form of change tracking or
another. Working with a plaintext source format such as LaTeX is always
best, of course. My only request is then, "diffs please".

The rendered format is absolutely welcome in addition to the diffs, of
course.

Thanks
Laszlo



Please let me know if you have any questions/comments/concerns.
thanks,
greg

pon., 19 kwi 2021 o 11:07 Grzegorz Bernacki <gjb@semihalf.com>
napisał(a):

Hi Laszlo,

Thanks a lot for your review. I will wait some time for more comments
and I will send an updated design.
thanks,
greg

pt., 16 kwi 2021 o 14:16 Laszlo Ersek <lersek@redhat.com> napisał(a):

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application
for
enrolling Secure Boot keys. The application uses content of
PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in
flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing

- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea
to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at
least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature
enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo




Laszlo Ersek
 

On 04/29/21 09:17, Grzegorz Bernacki wrote:
Hi Laszlo,

Please find diff at following link:
https://drive.google.com/file/d/1ZEzNUjaz9PiQPnQlIfsHYiJ6ZCda_szD/view?usp=sharing
Thank you; the updates look fine to me.

Laszlo


thanks,
greg

śr., 28 kwi 2021 o 18:55 Laszlo Ersek <lersek@redhat.com> napisał(a):

Hi Greg,

On 04/28/21 12:50, Grzegorz Bernacki wrote:
Hi,

Thanks a lot for reviewing my previous document. Please see below link to
updated design:
https://drive.google.com/file/d/11yNJJ2x8WXYZRg1nZWrr4C4Hq89Izn1D/view?usp=sharing

Changes:
- remove unnecessary description of default variables
- add macros to specify default keys files
- add fdf include file
- add UI to reset the content of the keys to default values
the master format for this document seems to be LaTeX (from the "pdf
properties" window in evince), which is fantastic: can you please post a
diff between the v1 and v2 LaTeX sources?

I can only deal with incremental reviews for v2 and later postings. Most
WYSIWYG document editors nowadays support one form of change tracking or
another. Working with a plaintext source format such as LaTeX is always
best, of course. My only request is then, "diffs please".

The rendered format is absolutely welcome in addition to the diffs, of
course.

Thanks
Laszlo



Please let me know if you have any questions/comments/concerns.
thanks,
greg

pon., 19 kwi 2021 o 11:07 Grzegorz Bernacki <gjb@semihalf.com>
napisał(a):

Hi Laszlo,

Thanks a lot for your review. I will wait some time for more comments
and I will send an updated design.
thanks,
greg

pt., 16 kwi 2021 o 14:16 Laszlo Ersek <lersek@redhat.com> napisał(a):

On 04/16/21 14:10, Laszlo Ersek wrote:
On 04/15/21 13:59, Grzegorz Bernacki wrote:
Hello,

I would like to ask you to review the proposal for new application
for
enrolling Secure Boot keys. The application uses content of
PKDefault,
KEKDefault, dbDefault, dbtDefault and dbxDefault to set Secure Boot
key. Default variables are initialized based on keys embedded in
flash
binary file. Please find details in following document:

https://drive.google.com/file/d/1rC6BjRWKODdXPdAa5cIeBG8_ye2L8aSZ/view?usp=sharing

- A new platform DXE driver (which need not even stay resident), for
locating some FFS files and their sections, and for copying their
contents into PKDefault and friends, seems like a good / useful idea
to me.

- Regarding the following sentence:

However there is no need to keep these variables as non-volatile in
order to avoid unnecessary bloating of the consumed non-volatile
storage space.
This is valid, but redundant (or at least somewhat redundantly
expressed), IMO. The UEFI spec already marks PKDefault as "BS, RT"
(section 3.3).

- The setting of PK from PKDefault (and similarly for the other
variables) is indeed a separate action. The UEFI spec says this is
primarily an OS action (again section 3.3), but I agree having a
dedicated UEFI app for this may be useful.

- Regarding the following sentence:

In case when default variables are already initialized a warning
message will be printed and variables will not be changed.

A warning is possible perhaps via status code reporting, or DEBUG. A
platform DXE driver is not supposed to write to the console.

- The idea as a whole looks good to me. I agree it should be testable
with OVMF, provided the FDF file(s) are modified as needed; however, I
wouldn't like to have that in the FDF file(s) permanently -- or at
least
it should be gated with a new build time feature test macro.

The

SECTION RAW = ...

statements in the FDF files could perhaps refer to macros as well (for
the pathnames). So a user building a platform with this feature
enabled
would have to pass a -D flag (boolean) for enabling the feature in
general (enabling the necessary lines in both the DSC and the FDF
file(s)), and then further -D flags for specifying the pathnames.
You could perhaps introduce DSC and FDF snippets (include files) too,
under SecurityPkg, so that any platform utilizing this feature do so
through identical macro names.

Thanks
Laszlo