Date
1 - 8 of 8
[PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation
|
Ard Biesheuvel
Expose the EFI_RNG_PROTOCOL based on RdRand, so that we don't have to
rely on QEMU providing a virtio-rng device in order to implement this protocol. Signed-off-by: Ard Biesheuvel <ardb@...> --- OvmfPkg/OvmfPkgIa32.dsc | 1 + OvmfPkg/OvmfPkgIa32.fdf | 1 + OvmfPkg/OvmfPkgIa32X64.dsc | 1 + OvmfPkg/OvmfPkgIa32X64.fdf | 1 + OvmfPkg/OvmfPkgX64.dsc | 1 + OvmfPkg/OvmfPkgX64.fdf | 1 + 6 files changed, 6 insertions(+) diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index e9ba491237ae..18c1e7255812 100644 --- a/OvmfPkg/OvmfPkgIa32.dsc +++ b/OvmfPkg/OvmfPkgIa32.dsc @@ -941,6 +941,7 @@ [Components] }=0D !endif=0D =0D + SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf=0D !if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDx= e.inf=0D OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf=0D diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index 7023ade8cebe..34f27ca832bc 100644 --- a/OvmfPkg/OvmfPkgIa32.fdf +++ b/OvmfPkg/OvmfPkgIa32.fdf @@ -248,6 +248,7 @@ [FV.DXEFV] INF OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf=0D !endif=0D =0D + INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf=0D !if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon= figDxe.inf=0D !endif=0D diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc index af566b953f36..e9a199c9f490 100644 --- a/OvmfPkg/OvmfPkgIa32X64.dsc +++ b/OvmfPkg/OvmfPkgIa32X64.dsc @@ -955,6 +955,7 @@ [Components.X64] }=0D !endif=0D =0D + SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf=0D !if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDx= e.inf=0D OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf=0D diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf index 80de4fa2c0df..33cc163e596e 100644 --- a/OvmfPkg/OvmfPkgIa32X64.fdf +++ b/OvmfPkg/OvmfPkgIa32X64.fdf @@ -249,6 +249,7 @@ [FV.DXEFV] INF OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf=0D !endif=0D =0D + INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf=0D !if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon= figDxe.inf=0D !endif=0D diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index f39d9cd117e6..5572cb82998f 100644 --- a/OvmfPkg/OvmfPkgX64.dsc +++ b/OvmfPkg/OvmfPkgX64.dsc @@ -1023,6 +1023,7 @@ [Components] }=0D !endif=0D =0D + SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf=0D !if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDx= e.inf=0D OvmfPkg/EnrollDefaultKeys/EnrollDefaultKeys.inf=0D diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index c0f5a1ef3c30..d42deebe3f8f 100644 --- a/OvmfPkg/OvmfPkgX64.fdf +++ b/OvmfPkg/OvmfPkgX64.fdf @@ -274,6 +274,7 @@ [FV.DXEFV] INF OvmfPkg/LsiScsiDxe/LsiScsiDxe.inf=0D !endif=0D =0D +INF SecurityPkg/RandomNumberGenerator/RngDxe/RngDxe.inf=0D !if $(SECURE_BOOT_ENABLE) =3D=3D TRUE=0D INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootCon= figDxe.inf=0D !endif=0D --=20 2.35.1 |
|
|
|
Pedro Falcato
Hi Ard, Given this patch plus the corresponding linux-efi patches wrt RNG, I'm mildly concerned about buggy RDRAND implementations compromising the kernel's RNG. Is this not a concern? It's also worth noting that MdePkg/Library/BaseRngLib skips the CPUID bit check in ArchIsRngSupported for $REASON, which I assume will crash pre-RDRAND VMs. We should probably also test for stupidly broken rdrand implementations like the notorious Zen 3 which always return 0xFFFFFFFF (per xkcd 221 ;)). Thanks, Pedro On Thu, Nov 10, 2022 at 1:48 PM Ard Biesheuvel <ardb@...> wrote: Expose the EFI_RNG_PROTOCOL based on RdRand, so that we don't have to -- Pedro Falcato |
|
|
|
Pedro Falcato
On Tue, Nov 22, 2022 at 12:20 PM Jason A. Donenfeld <Jason@...> wrote: Hi Pedro, Hi, Thank you for your thorough response, glad to have you in this thread. Speaking with my kernel RNG maintainer hat on, no, this is not really a I am aware, but I'm more scared when it comes to very early boot (think linux's EFI stub or some other bootloader) I can see how an ill-advised RNG_PROTOCOL user can try to exclusively rely on it (if it's available, which I don't believe it is atm on non-virtio-rng OVMF) vs mixing in the few sources you can get at that point, making important things like KASLR addresses or possibly even a stack canary 100% guessable. - The kernel will use RDRAND on its own, even in the case that EFI Yes, but as you said, the kernel mixes RDRAND with a lot of other entropy sources and also does proper sanity checking on it. - EFI on actual baremetal firmware, as opposed to OVMF, already provides Hm? What do you mean? - Most of those RDRAND bugs have concerned coming in and out of various Ah yes, I haven't been paying much close attention to the RNG patches themselves but now that I took a closer look I can see you're right. As a general point, this question of "do we have enough entropy?" or I know, I'm not yelling at Ard for the (questionable?) choices done in the BaseRngLib code, but I'm concerned this patch may negatively influence any sort of early boot RNG, particularly for the more naive users of RNG_PROTOCOL, by providing the possibly flawed RDRAND code. If the efi subsystem/EFISTUB code handles this case well by still mixing in whatever sources it can get before using this entropy, then that's great, but providing things like a non-random RNG_PROTOCOL sounds very broken and very unsafe to me (again invoking that possible KASLR at very early boot example). Also the CPUID check seems like an important step towards not-breaking-old-CPUs. All I'm saying is that we shouldn't just hook up the RNG DXE driver without carefully considering what the code is doing. Thanks, Pedro |
|
|
|
Pedro Falcato
On Tue, Nov 22, 2022 at 1:10 PM Jason A. Donenfeld <Jason@...> wrote: Yes. If what you say is true, this should be fixed asap. Do you intend I have sent out a patch (https://edk2.groups.io/g/devel/message/96552) fixing the CPUID checks with a naive attempt to sniff out RDRAND issues. Your Linux snippet is probably better but I couldn't look at it due to licensing concerns. To point out again, this is already hooked up to baremetal firmware. "Real firmware" is frequently not a shining example of quality :) They can also more or less know what CPUs the fw will run on, but OVMF runs mostly everywhere. In any case, I've sent out a patch (hopefully) fixing these issues. It should be trivial enough to review and apply so that Ard's patch doesn't get held up. I think it's best to apply this only after my patch gets merged (in order to avoid weird breakage between commits) but that's up to the OVMF maintainers to decide. Pedro |
|
|
|
Jason A. Donenfeld <Jason@...>
Hi Pedro,
On Tue, Nov 22, 2022 at 12:35 PM Pedro Falcato <pedro.falcato@...> wrote: Given this patch plus the corresponding linux-efi patches wrt RNG, I'mSpeaking with my kernel RNG maintainer hat on, no, this is not really a concern, for several reasons: - The kernel's RNG takes input from multiple sources, continuously, and tries to mix in new inputs rather quickly, especially at early boot. - The kernel will use RDRAND on its own, even in the case that EFI doesn't provide it, so it's not gaining anything here. - EFI on actual baremetal firmware, as opposed to OVMF, already provides EFI, so this is par for the course. - Most of those RDRAND bugs have concerned coming in and out of various sleep states, which doesn't really apply to early boot EFI. - And again, just to reinforce the first point, the kernel takes inputs from many sources. Having EFI provide its own thing -- via RDRAND or any other mechanism -- is complementary and will only help. Regarding your "corresponding linux-efi patches wrt RNG", I'm not quite sure what you're referring to. If anything, recent work during this cycle has been aimed around shuffling more sources of randomness in from elsewhere. The EFI RNG protocol stuff has been in there already for a long time. So maybe you misunderstood those? Or I'm misunderstanding what you're referring to? As a general point, this question of "do we have enough entropy?" or "are we initialized yet?" is an impossible proposition. Entropy estimation is impossible, and is only ever a guess, and that guess can be sometimes wrong, even wildly wrong. So the kernel is increasingly moving away from /relying/ on that, and is more focused on getting more sources faster -- incorporating anything it can find, and mixing it into the output stream more continuously. To that end, if EFI's got a DXE to do something or another, please hook it up. Lastly, I think the concern you raised is inappropriate for Ard's patchset, as it actually doesn't apply to it at all. This patchset is about hooking an existing DXE up to OVMF, one that is hooked up elsewhere, but wasn't for OVMF. This alone has nothing to do with the concern. Rather, the concerns you raised are about the DXE itself. So to that end, perhaps you should start a new thread or send some patches or do something to the DXE that you're concerned about (e.g. a basic boring power-on selftest like what the kernel has or something, if you're extra worried). Or maybe not, for the reasons I listed above of things being basically fine. Jason |
|
|
|
Jason A. Donenfeld <Jason@...>
Hi again,
On Tue, Nov 22, 2022 at 11:35:06AM +0000, Pedro Falcato wrote: We should probably also test for stupidly broken rdrand implementationsOn this topic, if you did want to improve this part of that DXE, the kernel's test for this is super dumb and basic but I think basically gets the job done for the most pathological scenarios. From arch/x86/kernel/cpu/rdrand.c: void x86_init_rdrand(struct cpuinfo_x86 *c) { enum { SAMPLES = 8, MIN_CHANGE = 5 }; unsigned long sample, prev; bool failure = false; size_t i, changed; if (!cpu_has(c, X86_FEATURE_RDRAND)) return; for (changed = 0, i = 0; i < SAMPLES; ++i) { if (!rdrand_long(&sample)) { failure = true; break; } changed += i && sample != prev; prev = sample; } if (changed < MIN_CHANGE) failure = true; if (failure) { clear_cpu_cap(c, X86_FEATURE_RDRAND); clear_cpu_cap(c, X86_FEATURE_RDSEED); pr_emerg("RDRAND is not reliable on this platform; disabling.\n"); } } Feel free to lift that into the DXE if you want. That should probably be a different patch to this series, though, as I mentioned in my last email. The CPUID check that you mentioned, however, seems like an important prerequisite to this series. Jason |
|
|
|
Jason A. Donenfeld <Jason@...>
Hi Pedro,
On 11/22/22, Pedro Falcato <pedro.falcato@...> wrote: I am aware, but I'm more scared when it comes to very early boot (thinkThe thing is, in low-entropy early boot scenarios, the alternative would be just having nothing. The kernel doesn't pause boot to wait for a good source....it just uses a bad one. So adding RDRAND in EFI helps. Really, more entropy from more sources as early as possible and as fast as possible only ever helps here. also does proper sanity checking on it.No, there's nothing "proper" about it. It's a dumb basic thing. I meant already provides RDRAND, sorry. All x86 hardware with EFI has this enabled. I know, I'm not yelling at Ard for the (questionable?) choices done in theExcept as related several times now, it will only help and won't hurt. If you want to improve the RDRAND DXE, do it, but aside from the CPUID issue you raised, I don't think your "concerns" warrant holding up this patch. Yes. If what you say is true, this should be fixed asap. Do you intend to send a patch? All I'm saying is that we shouldn't just hook up the RNG DXE driver withoutTo point out again, this is already hooked up to baremetal firmware. So if you see issues that are worth fixing, fix them, but it shouldn't hold up Ard's patchset. Jason |
|
|
|
Jason A. Donenfeld
Hi,
On Tue, Nov 22, 2022 at 3:17 PM Pedro Falcato <pedro.falcato@...> wrote: I have sent out a patch (https://edk2.groups.io/g/devel/message/96552) fixing the CPUID checks with a naive attempt to sniff out RDRAND issues.I (re)wrote that function in Linux. I hereby relicense it as MIT, and also place it into public domain. Do with it what you will now. Jason |
|
|