Re: [edk2-devel] [PATCH 3/3] OvmfPkg/OvmfX86: Enable RDRAND based EFI_RNG_PROTOCOL implementation

["Jason A. Donenfeld" <Jason@zx2c4.com>]

Hi Pedro,

On 11/22/22, Pedro Falcato <pedro.falcato@gmail.com> wrote:
> 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 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.


>
> - EFI on actual baremetal firmware, as opposed to OVMF, already provides
>>   EFI, so this is par for the course.
>>
>
> Hm? What do you mean?

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 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).

Except 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.

>
> Also the CPUID check seems like an important step towards
> not-breaking-old-CPUs.

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 without
> carefully considering what the code is doing.

To 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