On Wed, 9 Nov 2022 at 11:34, Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
>
> Hi Ard,
>
> Looking good! Thanks. A few brief comments.
>
> On Wed, Nov 09, 2022 at 10:55:58AM +0100, Ard Biesheuvel wrote:
> > The recommended seed
> > size is 32 bytes, anything beyond that is disregarded when the seeds are
> > concatenated.
>
> This should read, "The recommended seed size is 32 bytes, and seeds
> larger than 512 bytes are considered corrupted and ignored entirely."
>
Ack.
> > Cc: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx>
> > Cc: Jason A. Donenfeld <Jason@xxxxxxxxx>
> > Cc: Lennart Poettering <lennart@xxxxxxxxxxxxxx>
> > Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
>
> I was thinking it might be best to add:
>
> Cc: stable@xxxxxxxxxxxxxxx
>
> "Don't clobber existing GUID" seems like it's arguably a fix.
>
Yeah, I'm on the fence about that one. I'll leave it out for now, and
we can always get it into -stable later. I am not going to queue it as
a fix now in any case.
> > status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
> > - sizeof(*seed) + EFI_RANDOM_SEED_SIZE,
> > + struct_size(seed, bits, seed_size),
> > (void **)&seed);
> > if (status != EFI_SUCCESS)
> > return status;
>
> Should this print "Failed to something something, retaining
> bootloader-supplied seed only", like the err_freepool case?
>
Yeah, it's just something that is exceedingly unlikely to ever occur
in practice, and if it does, whether or not the efi_warn() will
produce any observable output is anyone's guess.
But i'll add it just for good measure.
> > + efi_warn("Failed to obtain seed from EFI_RNG_PROTOCOL%s\n",
> > + prev_seed ? ", retaining bootloader supplied seed only" : "");
>
> "bootloader-supplied" with the hyphen, right?
>
Ack.
> If you make any of the above changes, feel free to do it when you commit
> this. IOW, my comments are nits, so:
>
> Reviewed-by: Jason A. Donenfeld <Jason@xxxxxxxxx>
>
Thanks.
> Also, I verified that kexec correctly sets everything back to 32 bytes
> with this line:
> size = min(seed->size, EFI_RANDOM_SEED_SIZE);
> So that's good.
>
> Jason
