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

2018-01-23 Thread Ard Biesheuvel
On 12 January 2018 at 11:24, Daniel Kiper  wrote:
> Hi Ard,
>
> On Thu, Jan 11, 2018 at 12:51:07PM +, Ard Biesheuvel wrote:
>> On 9 January 2018 at 14:22, Daniel Kiper  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] efi/arm*: Only register page tables when they exist

2018-01-23 Thread Ard Biesheuvel
On 17 January 2018 at 12:01, Ard Biesheuvel  wrote:
> On 17 January 2018 at 10:19, Will Deacon  wrote:
>> On Tue, Jan 16, 2018 at 02:08:32PM +, Mark Rutland wrote:
>>> Currently the arm/arm64 runtime code registers the runtime servies
>>> pagetables with ptdump regardless of whether runtime services page
>>> tables have been created.
>>>
>>> As efi_mm.pgd is NULL in these cases, attempting to dump the efi page
>>> tables results in a NULL pointer dereference in the ptdump code:
>>
>> Acked-by: Will Deacon 
>>
>> Ard -- please can you pick this one up?
>>
>
> I'll see if I can manage applying it.
>

Applied for -next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v1] efi/apple-properties: Device core takes care of empty properties

2018-01-23 Thread Ard Biesheuvel
On 22 January 2018 at 21:46, Lukas Wunner  wrote:
> On Mon, Jan 22, 2018 at 04:17:09PM +0200, Andy Shevchenko wrote:
>> There is no need to artificially supply a property length and fake data
>> if property has type of boolean.
>
> You're right, I just didn't know better.
>
> I've verified that with this patch applied, boolean properties are
> still listed with the "dump_apple_properties" command line option
> and I've also tested that it's still possible to test presence of
> boolean properties with device_property_present(), so FWIW:
>
> Reviewed-and-tested-by: Lukas Wunner 
>
> Thanks Andy!  Ard, could you pick this up please?
>

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] x86/efi: Fix trailing semicolons

2018-01-23 Thread Ard Biesheuvel
On 23 January 2018 at 13:20, Luis de Bethencourt  wrote:
> The trailing semicolon is an empty statement that does no operation.
> Removing them since they don't do anything.
>
> Signed-off-by: Luis de Bethencourt 
> ---
>
> Hi,
>
> After fixing the same thing in drivers/staging/rtl8723bs/, Joe Perches
> suggested I fix it treewide [0].
>
> Best regards
> Luis
>

Applied, thanks.


>
> [0] 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115410.html
> [1] 
> http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115390.html
>
>  arch/x86/boot/compressed/eboot.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/boot/compressed/eboot.c 
> b/arch/x86/boot/compressed/eboot.c
> index 353e20c3f114..886a9115af62 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -439,7 +439,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 
> *width, u32 *height)
> struct efi_uga_draw_protocol *uga = NULL, *first_uga;
> efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
> unsigned long nr_ugas;
> -   u32 *handles = (u32 *)uga_handle;;
> +   u32 *handles = (u32 *)uga_handle;
> efi_status_t status = EFI_INVALID_PARAMETER;
> int i;
>
> @@ -484,7 +484,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 
> *width, u32 *height)
> struct efi_uga_draw_protocol *uga = NULL, *first_uga;
> efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
> unsigned long nr_ugas;
> -   u64 *handles = (u64 *)uga_handle;;
> +   u64 *handles = (u64 *)uga_handle;
> efi_status_t status = EFI_INVALID_PARAMETER;
> int i;
>
> --
> 2.15.1
>
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH] x86/efi: Fix trailing semicolons

2018-01-23 Thread Luis de Bethencourt
The trailing semicolon is an empty statement that does no operation.
Removing them since they don't do anything.

Signed-off-by: Luis de Bethencourt 
---

Hi,

After fixing the same thing in drivers/staging/rtl8723bs/, Joe Perches
suggested I fix it treewide [0].

Best regards 
Luis


[0] 
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115410.html
[1] 
http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/2018-January/115390.html

 arch/x86/boot/compressed/eboot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 353e20c3f114..886a9115af62 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -439,7 +439,7 @@ setup_uga32(void **uga_handle, unsigned long size, u32 
*width, u32 *height)
struct efi_uga_draw_protocol *uga = NULL, *first_uga;
efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
unsigned long nr_ugas;
-   u32 *handles = (u32 *)uga_handle;;
+   u32 *handles = (u32 *)uga_handle;
efi_status_t status = EFI_INVALID_PARAMETER;
int i;
 
@@ -484,7 +484,7 @@ setup_uga64(void **uga_handle, unsigned long size, u32 
*width, u32 *height)
struct efi_uga_draw_protocol *uga = NULL, *first_uga;
efi_guid_t uga_proto = EFI_UGA_PROTOCOL_GUID;
unsigned long nr_ugas;
-   u64 *handles = (u64 *)uga_handle;;
+   u64 *handles = (u64 *)uga_handle;
efi_status_t status = EFI_INVALID_PARAMETER;
int i;
 
-- 
2.15.1

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html