linux-kernel - Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAKv+Gu8P6aRhGruBfXTauTDNgCF00SV2PmCDC+=ZNmoYoeVciA@mail.gmail.com>
Date:   Tue, 23 Jan 2018 19:23:02 +0000
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Daniel Kiper <daniel.kiper@...cle.com>
Cc:     Ingo Molnar <mingo@...hat.com>, linux-efi@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        xen-devel <xen-devel@...ts.xenproject.org>,
        Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        "H. Peter Anvin" <hpa@...or.com>, Juergen Gross <jgross@...e.com>,
        Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during
 dom0 boot

On 12 January 2018 at 11:24, Daniel Kiper <daniel.kiper@...cle.com> wrote:
> Hi Ard,
>
> On Thu, Jan 11, 2018 at 12:51:07PM +0000, Ard Biesheuvel wrote:
>> On 9 January 2018 at 14:22, Daniel Kiper <daniel.kiper@...cle.com> wrote:
>> > Hi,
>> >
>> > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
>> > may not even know that it runs on secure boot enabled platform.
>>
>> Hi Daniel,
>>
>> I must say, I am not too thrilled with the approach you have chosen
>> here. #including .c files in other .c files, and using #defines to
>> override C functions or other stub functionality is rather fragile. In
>
> TBH I do not like it too. Sadly I have not find a better solution for
> that. I wish to avoid code duplication as much as possible because
> otherwise it will fall out of sync sooner or later (usually sooner).
> Similar thing happened in different part of Xen EFI code a few months ago.
>
>> particular, it means we have to start caring about not breaking
>> Xen/x86 code when making modifications to the EFI stub, and that code
>> is already difficult enough to maintain, given that it is shared
>> between ARM, arm64 and x86, and runs either from the decompressor or
>> the kernel proper (arm64) but in the context of the UEFI firmware.
>
> I understand that.
>
>> None of the stub code currently runs in ordinary kernel context.
>
> Yep.
>
>> So please, could you try to find another way to do this?
>
> I am happy to improve the situation, however, I am afraid that it is
> difficult here. Stub and kernel proper are separate entities and simple
> linking does not work. So, It seems to me that only play with includes
> will allow us to not duplicate the code. However, if you have a better
> idea I am happy to implement it.
>

Actually, there is another reason why it does not make sense to reuse that code.

This code

/*
* See if a user has put the shim into insecure mode. If so, and if the
* variable doesn't have the runtime attribute set, we might as well
* honor that.
*/
size = sizeof(moksbstate);
status = get_efi_var(L"MokSBState", &shim_guid,
     &attr, &size, &moksbstate);

/* If it fails, we don't care why. Default to secure */
if (status != EFI_SUCCESS)
    goto secure_boot_enabled;
if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
    return efi_secureboot_mode_disabled;

will always fail after exiting boot services, so it makes no sense to
call it from xen_efi_init().

So I suggest you just clone the function and only keep the pieces that
make sense for Xen.

-- 
Ard.