Re: [PATCH v2 1/2] x86: only modify setup_data if the boot protocol indicates safety

[Laszlo Ersek]

On 09/06/22 13:33, Daniel P. Berrangé wrote:
> On Tue, Sep 06, 2022 at 01:14:50PM +0200, Ard Biesheuvel wrote:
>> (cc Laszlo)
>>
>> On Tue, 6 Sept 2022 at 12:45, Michael S. Tsirkin <mst@redhat.com> wrote:
>>>
>>> On Tue, Sep 06, 2022 at 12:43:55PM +0200, Jason A. Donenfeld wrote:
>>>> On Tue, Sep 6, 2022 at 12:40 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>
>>>>> On Tue, Sep 06, 2022 at 12:36:56PM +0200, Jason A. Donenfeld wrote:
>>>>>> It's only safe to modify the setup_data pointer on newer kernels where
>>>>>> the EFI stub loader will ignore it. So condition setting that offset on
>>>>>> the newer boot protocol version. While we're at it, gate this on SEV too.
>>>>>> This depends on the kernel commit linked below going upstream.
>>>>>>
>>>>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>>>>> Cc: Laurent Vivier <laurent@vivier.eu>
>>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>>>> Cc: Peter Maydell <peter.maydell@linaro.org>
>>>>>> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> Cc: Richard Henderson <richard.henderson@linaro.org>
>>>>>> Cc: Ard Biesheuvel <ardb@kernel.org>
>>>>>> Link: 
>>>>>> https://lore.kernel.org/linux-efi/20220904165321.1140894-1-Jason@zx2c4.com/
>>>>>> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
>>>>>
>>>>> BTW what does it have to do with SEV?
>>>>> Is this because SEV is not going to trust the data to be random anyway?
>>>>
>>>> Daniel (now CC'd) pointed out in one of the previous threads that this
>>>> breaks SEV, because the image hash changes.
>>>>
>>>> Jason
>>>
>>> Oh I see. I'd add a comment maybe and definitely mention this
>>> in the commit log.
>>>
>>
>> This does raise the question (as I mentioned before) how things like
>> secure boot and measured boot are affected when combined with direct
>> kernel boot: AIUI, libvirt uses direct kernel boot at guest
>> installation time, and modifying setup_data will corrupt the image
>> signature.
> 
> IIUC, qemu already modifies setup_data when using direct kernel boot.
> 
> It put in logic to skip this if SEV is enabled, to avoid interfering
> with SEV hashes over the kernel, but there's nothing doing this more
> generally for non-SEV cases using UEFI. So potentially use of SecureBoot
> may already be impacted when using direct kernel boot.

Yes,

https://github.com/tianocore/edk2/commit/82808b422617

Laszlo

> I haven't formally
> tested this myself though. I just saw that earlier versions of this
> RNG patch broke SEV hashes and later versions addressed that problem
> for SEV when the code was re-arranged.
> 
> With regards,
> Daniel
>