[PATCH v4 3/5] OvmfPkg: Add RngLib based on TimerLib for Crypto


Matthew Carlson
 

From: Matthew Carlson <macarl@...>

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Anthony Perard <anthony.perard@...>
Cc: Julien Grall <julien@...>
Signed-off-by: Matthew Carlson <matthewfcarlson@...>
---
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
4 files changed, 4 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 9178ffeb71cb..118fd1aff246 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -116,6 +116,7 @@
[LibraryClasses]=0D
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf=0D
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf=0D
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf=0D
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf=0D
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf=0D
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf=
=0D
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a665f78f0dc7..6b9da5b996ff 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -120,6 +120,7 @@
[LibraryClasses]=0D
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf=0D
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf=0D
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf=0D
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf=0D
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf=0D
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf=
=0D
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 17f345acf4ee..3a354eb3a2bd 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -120,6 +120,7 @@
[LibraryClasses]=0D
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf=0D
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf=0D
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf=0D
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf=0D
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf=0D
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf=
=0D
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 782803cb2787..f97e2b7e07d0 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -110,6 +110,7 @@
[LibraryClasses]=0D
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf=0D
TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf=0D
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf=0D
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf=0D
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf=0D
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf=
=0D
--=20
2.27.0.windows.1


Ard Biesheuvel
 

On 8/11/20 4:21 AM, matthewfcarlson@... wrote:
From: Matthew Carlson <macarl@...>
How am I supposed to review this change? The commit log is empty and I was not cc'ed on the cover letter.

In general, please try to muster up the energy to write at least one sentence that describes *why* the patch is needed, complementing the subject line, which in this case summarizes correctly *what* the patch does.

Thanks,
Ard.



Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Anthony Perard <anthony.perard@...>
Cc: Julien Grall <julien@...>
Signed-off-by: Matthew Carlson <matthewfcarlson@...>
---
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
4 files changed, 4 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 9178ffeb71cb..118fd1aff246 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -116,6 +116,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a665f78f0dc7..6b9da5b996ff 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -120,6 +120,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 17f345acf4ee..3a354eb3a2bd 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -120,6 +120,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 782803cb2787..f97e2b7e07d0 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -110,6 +110,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf


Laszlo Ersek
 

Hi Ard!

On 08/11/20 10:22, Ard Biesheuvel wrote:
On 8/11/20 4:21 AM, matthewfcarlson@... wrote:
From: Matthew Carlson <macarl@...>
How am I supposed to review this change? The commit log is empty and I
was not cc'ed on the cover letter.
Cover letter:

[edk2-devel] [PATCH v4 0/5] Use RngLib instead of TimerLib for OpensslLib

https://edk2.groups.io/g/devel/message/63944
http://mid.mail-archive.com/20200811022200.1087-1-matthewfcarlson@gmail.com

Bugzilla:

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

Unfortunately, the cover letter doesn't much explain the approach
either. The latest comments in the BZ should be helpful though.

My understanding is that the timer-based "pseudo-random" generation is
factored out of "CryptoPkg/Library/OpensslLib/rand_pool_noise*" to the
new BaseRngLibTimerLib instance (see patches #1 and #5). In the middle,
platforms native to the edk2 tree and currently using "rand_pool_noise*"
are diverted to the new lib instance. (Patches #3 and #4.)

So I think the intent is to introduce no change in behavior for those
platforms, only make OpensslLib depend on the RngLib class.

Patch#2 adds BaseRngLibDxe, which depends on gEfiRngProtocolGuid.

I think the structure of the series is correct.

--*--

In edk2, we have two RNG protocol implementations,
"OvmfPkg/VirtioRngDxe" and "SecurityPkg/RandomNumberGenerator/RngDxe".
While it would be nice to use the "BaseRngLibDxe" instance in OvmfPkg
and ArmVirtPkg, *in the longer term*, I have some doubts:

- I don't know whether or how "SecurityPkg/RandomNumberGenerator/RngDxe"
applies to virtual machines.

- OvmfPkg/VirtioRngDxe does not produce gEfiRngProtocolGuid if there is
no virtio-rng-(pci|device) device configured in QEMU. So a strict depex
would not work; we'd again need some kind of OR depex.

- The ArmVirtQemu and OVMF PlatformBootManagerLib instances connect
virtio-rng-(pci|device) devices after signaling EndOfDxe. That's good
enough for boot loaders and the Linux kernel's UEFI stub, but possibly
not good enough for platform DXE drivers that need randomness before
EndOfDxe.

- The "BaseRngLibDxe" instance from patch#2 only accepts one of the
"Sp80090Ctr256", "Sp80090Hmac256", and "Sp80090Hash256" algorithms, and
"OvmfPkg/VirtioRngDxe" provides none of those.
("SecurityPkg/RandomNumberGenerator/RngDxe" seems to provide
"Sp80090Ctr256".)

But, anyway, these are just longer-term points for OvmfPkg and
ArmVirtPkg; they aren't a problem with this patch set.

In general, please try to muster up the energy to write at least one
sentence that describes *why* the patch is needed, complementing the
subject line, which in this case summarizes correctly *what* the patch
does.
Agreed.

And, in addition to the minimally one-sentence commit message body, each
commit message should reference
<https://bugzilla.tianocore.org/show_bug.cgi?id=1871>.


I'd be very happy if you could review this patch series; personally I
can only formally review patches #3 and #4.

Thanks!
Laszlo


Laszlo Ersek
 

On 08/11/20 04:21, Matthew Carlson wrote:
From: Matthew Carlson <macarl@...>

Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Anthony Perard <anthony.perard@...>
Cc: Julien Grall <julien@...>
Signed-off-by: Matthew Carlson <matthewfcarlson@...>
---
OvmfPkg/OvmfPkgIa32.dsc | 1 +
OvmfPkg/OvmfPkgIa32X64.dsc | 1 +
OvmfPkg/OvmfPkgX64.dsc | 1 +
OvmfPkg/OvmfXen.dsc | 1 +
4 files changed, 4 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 9178ffeb71cb..118fd1aff246 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -116,6 +116,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a665f78f0dc7..6b9da5b996ff 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -120,6 +120,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 17f345acf4ee..3a354eb3a2bd 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -120,6 +120,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseAcpiTimerLib.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 782803cb2787..f97e2b7e07d0 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -110,6 +110,7 @@
[LibraryClasses]
PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
TimerLib|MdePkg/Library/SecPeiDxeTimerLibCpu/SecPeiDxeTimerLibCpu.inf
+ RngLib|MdePkg/Library/BaseRngLibTimerLib/BaseRngLibTimerLib.inf
ResetSystemLib|OvmfPkg/Library/ResetSystemLib/BaseResetSystemLib.inf
PrintLib|MdePkg/Library/BasePrintLib/BasePrintLib.inf
BaseMemoryLib|MdePkg/Library/BaseMemoryLibRepStr/BaseMemoryLibRepStr.inf
(1) This patch does not cover "OvmfPkg/Bhyve/BhyvePkgX64.dsc", which
also resolves "OpensslLib".

(2) Please add the RngLib resolution just after the "OpensslLib"
resolution(s), in each of the five DSC files.

Thank you,
Laszlo


Matthew Carlson
 

Thank you for the helpful comments Lazlo!

I sent out an updated series (v5) that fixes the things you mentioned. I added more description, so hopefully that helps.

Sorry I didn't notice BhyvePkg, I thought you couldn't have packages under other packages, so I didn't think to check for other DSC's. It should be fixed up like the other Ovmf DSC's.

I've been following your excellent guide for sending mailing list patches (Lazlo's Guide). Is there a better way to get all the CC's from all the patches other than just copy and pasting them all? Perhaps the GetMaintainers.py where you specify multiple commits? Specifying a range didn't produce the desired behavior.

--
- Matthew Carlson


Laszlo Ersek
 

(+Rebecca)

On 08/12/20 04:27, macarl via [] wrote:
Thank you for the helpful comments Lazlo!

I sent out an updated series (v5) that fixes the things you mentioned. I added more description, so hopefully that helps.

Sorry I didn't notice BhyvePkg, I thought you couldn't have packages under other packages, so I didn't think to check for other DSC's. It should be fixed up like the other Ovmf DSC's.
Right, with Bhyve we indeed broke the DEC spec first, having two DEC
files under OvmfPkg (this was reported by Sean). The issue was fixed in
commit e557442e3f7e; since then, bhyve is not a new package nested under
OvmfPkg (which is invalid), just a separate platform DSC.

Arguably, the "Pkg" infix in the following file names:

Bhyve/BhyvePkgDefines.fdf.inc
Bhyve/BhyvePkgX64.dsc
Bhyve/BhyvePkgX64.fdf

remains a bit confusing, and should indeed be removed:

Bhyve/BhyveDefines.fdf.inc
Bhyve/BhyveX64.dsc
Bhyve/BhyveX64.fdf

Rebecca, could you please submit a patch with such renames?

I've been following your excellent guide for sending mailing list patches (Lazlo's Guide). ( https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers ) Is there a better way to get all the CC's from all the patches other than just copy and pasting them all? Perhaps the GetMaintainers.py where you specify multiple commits? Specifying a range didn't produce the desired behavior.
For collecting the full CC set for the cover letter, one possibility is:

$ git log master..topic \
| grep '^Cc:' \
| sort -u

and then cut n' paste the result of this command into the cover letter.

(I assume even on Windows the above command should work in WSL or Cygwin.)

Thanks!
Laszlo


Rebecca Cran
 

On 8/12/20 4:05 AM, Laszlo Ersek wrote:

Arguably, the "Pkg" infix in the following file names:

Bhyve/BhyvePkgDefines.fdf.inc
Bhyve/BhyvePkgX64.dsc
Bhyve/BhyvePkgX64.fdf

remains a bit confusing, and should indeed be removed:

Bhyve/BhyveDefines.fdf.inc
Bhyve/BhyveX64.dsc
Bhyve/BhyveX64.fdf

Rebecca, could you please submit a patch with such renames?
Yes, I'll submit a patch in the next couple of days - I'm still catching up from traveling this past week.


--
Rebecca Cran