Re: [PATCH 0/6] efi/x86 mixed mode cleanups
On 14 July 2018 at 13:41, Lukas Wunner wrote: > On Fri, Jul 13, 2018 at 03:54:29PM +0200, Ard Biesheuvel wrote: >> Did you have any thoughts about how we could at least detect >> situations where we are invoking protocols that are not mixed-mode >> safe (i.e., they pass 64-bit quantities by value, which our current >> thunking routine does not take into account) > > efi_thunk_64.S only supports five arguments AFAICS, we'd have to check > for each argument passed to efi_call_early() or efi_call_proto() if > the corresponding argument in the *_32_t protocol struct has sizeof(u64), > and BUILD_BUG_ON() if so. > > Is it possible to retrieve the n-th element of a struct without knowing > its name? Basically offsetof() but with a number instead of a name? > We have to check the size of the struct element, not the argument passed > in to efi_call_early() / efi_call_proto() because we pass in 64-bit > pointers (and possibly also integers) and fully expect that the upper > 32-bit are disregarded. > > Difficult. :-( > Yeah. I think a GCC plugin is the only way to achieve this. -- 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 0/6] efi/x86 mixed mode cleanups
On Fri, Jul 13, 2018 at 03:54:29PM +0200, Ard Biesheuvel wrote: > Did you have any thoughts about how we could at least detect > situations where we are invoking protocols that are not mixed-mode > safe (i.e., they pass 64-bit quantities by value, which our current > thunking routine does not take into account) efi_thunk_64.S only supports five arguments AFAICS, we'd have to check for each argument passed to efi_call_early() or efi_call_proto() if the corresponding argument in the *_32_t protocol struct has sizeof(u64), and BUILD_BUG_ON() if so. Is it possible to retrieve the n-th element of a struct without knowing its name? Basically offsetof() but with a number instead of a name? We have to check the size of the struct element, not the argument passed in to efi_call_early() / efi_call_proto() because we pass in 64-bit pointers (and possibly also integers) and fully expect that the upper 32-bit are disregarded. Difficult. :-( Lukas -- 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 0/6] efi/x86 mixed mode cleanups
On 13 July 2018 at 11:57, Lukas Wunner wrote:
> On Thu, Jul 12, 2018 at 02:21:48PM +0200, Ard Biesheuvel wrote:
>> Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci
>> routine.
>>
>> Patches #3 and #4 do the same for the UGA draw protocol discovery routines.
>>
>> Patch #6 helps unused code paths to be optimized away on configurations
>> that don't need them (32-bit only and 64-bit only)
>
> Thanks a lot for doing this Ard, I meant to deduplicate that code
> further but didn't get around to it. FWIW I have a single unsubmitted
> deduplication patch which has been rotting on my development branch for
> more than a year now, I'm including it below in case it helps, needs a
> careful review though.
>
Thanks Lukas. I will apply that (with Ingo's comment addressed).
Did you have any thoughts about how we could at least detect
situations where we are invoking protocols that are not mixed-mode
safe (i.e., they pass 64-bit quantities by value, which our current
thunking routine does not take into account)
> -- >8 --
> From 7fa51faec20c74eea2bb3ba96ecab1bc3e18e5a8 Mon Sep 17 00:00:00 2001
> From: Lukas Wunner
> Date: Sun, 7 May 2017 14:42:51 +0200
> Subject: [PATCH] efi: Deduplicate efi_open_volume()
>
> There's one ARM, one x86_32 and one x86_64 version which can be folded
> into a single shared version by masking their differences with the
> efi_call_proto() macro introduced by commit 3552fdf29f01 ("efi: Allow
> bitness-agnostic protocol calls").
>
> To be able to dereference the device_handle attribute from the
> efi_loaded_image_t table in an arch- and bitness-agnostic manner,
> introduce the efi_table_attr() macro (which already exists for x86)
> to arm and arm64.
>
> No functional change intended.
>
> Signed-off-by: Lukas Wunner
> ---
> arch/arm/include/asm/efi.h | 3 ++
> arch/arm64/include/asm/efi.h | 3 ++
> arch/x86/boot/compressed/eboot.c | 61
> --
> drivers/firmware/efi/libstub/arm-stub.c| 25 ---
> drivers/firmware/efi/libstub/efi-stub-helper.c | 29 +++-
> drivers/firmware/efi/libstub/efistub.h | 3 --
> include/linux/efi.h| 10 +
> 7 files changed, 43 insertions(+), 91 deletions(-)
>
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 17f1f1a..38badaa 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -58,6 +58,9 @@ static inline void efi_set_pgd(struct mm_struct *mm)
> #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
> #define efi_is_64bit() (false)
>
> +#define efi_table_attr(table, attr, instance) \
> + ((table##_t *)instance)->attr
> +
> #define efi_call_proto(protocol, f, instance, ...) \
> ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
>
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index c4cd508..5a21037 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -85,6 +85,9 @@ static inline unsigned long
> efi_get_max_initrd_addr(unsigned long dram_base,
> #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
> #define efi_is_64bit() (true)
>
> +#define efi_table_attr(table, attr, instance) \
> + ((table##_t *)instance)->attr
> +
> #define efi_call_proto(protocol, f, instance, ...) \
> ((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
>
> diff --git a/arch/x86/boot/compressed/eboot.c
> b/arch/x86/boot/compressed/eboot.c
> index 9a5b6d3..2d2b599 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -41,67 +41,6 @@ __pure const struct efi_config *__efi_early(void)
> BOOT_SERVICES(32);
> BOOT_SERVICES(64);
>
> -static inline efi_status_t __open_volume32(void *__image, void **__fh)
> -{
> - efi_file_io_interface_t *io;
> - efi_loaded_image_32_t *image = __image;
> - efi_file_handle_32_t *fh;
> - efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
> - efi_status_t status;
> - void *handle = (void *)(unsigned long)image->device_handle;
> - unsigned long func;
> -
> - status = efi_call_early(handle_protocol, handle,
> - _proto, (void **));
> - if (status != EFI_SUCCESS) {
> - efi_printk(sys_table, "Failed to handle fs_proto\n");
> - return status;
> - }
> -
> - func = (unsigned long)io->open_volume;
> - status = efi_early->call(func, io, );
> - if (status != EFI_SUCCESS)
> - efi_printk(sys_table, "Failed to open volume\n");
> -
> - *__fh = fh;
> - return status;
> -}
> -
> -static inline efi_status_t __open_volume64(void *__image, void **__fh)
> -{
> - efi_file_io_interface_t *io;
> -
Re: [PATCH 0/6] efi/x86 mixed mode cleanups
On 13 July 2018 at 15:29, Ingo Molnar wrote:
>
> * Lukas Wunner wrote:
>
>> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
>> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
>> @@ -413,6 +413,32 @@ static efi_status_t efi_file_close(void *handle)
>> return efi_call_proto(efi_file_handle, close, handle);
>> }
>>
>> +static efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
>> + efi_loaded_image_t *image,
>> + efi_file_handle_t **__fh)
>> +{
>> + efi_file_io_interface_t *io;
>> + efi_file_handle_t *fh;
>> + efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
>> + efi_status_t status;
>> + void *handle =
>> + (void *)efi_table_attr(efi_loaded_image, device_handle, image);
>> +
>> + status = efi_call_early(handle_protocol, handle,
>> + _proto, (void **));
>> + if (status != EFI_SUCCESS) {
>> + efi_printk(sys_table_arg, "Failed to handle fs_proto\n");
>> + return status;
>> + }
>> +
>> + status = efi_call_proto(efi_file_io_interface, open_volume, io, );
>> + if (status != EFI_SUCCESS)
>> + efi_printk(sys_table_arg, "Failed to open volume\n");
>> +
>> + *__fh = fh;
>> + return status;
>
> BTW., in the second failure branch is 'fh' guaranteed to be set by the EFI
> call?
> If not then we set *__fh to a potentially undefined value, from the kernels
> stack?
>
> (I realize that your refactoring just inherited this existing pattern, but it
> caught my attention.)
>
No, only EFI_SUCCESS guarantees that fh will have been assigned.
Since we propagate 'status' here, it does not seem to matter in
practice, but it would indeed be better not to clobber *__fh with
random junk when returning failure.
--
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 0/6] efi/x86 mixed mode cleanups
* Lukas Wunner wrote:
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -413,6 +413,32 @@ static efi_status_t efi_file_close(void *handle)
> return efi_call_proto(efi_file_handle, close, handle);
> }
>
> +static efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
> + efi_loaded_image_t *image,
> + efi_file_handle_t **__fh)
> +{
> + efi_file_io_interface_t *io;
> + efi_file_handle_t *fh;
> + efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
> + efi_status_t status;
> + void *handle =
> + (void *)efi_table_attr(efi_loaded_image, device_handle, image);
> +
> + status = efi_call_early(handle_protocol, handle,
> + _proto, (void **));
> + if (status != EFI_SUCCESS) {
> + efi_printk(sys_table_arg, "Failed to handle fs_proto\n");
> + return status;
> + }
> +
> + status = efi_call_proto(efi_file_io_interface, open_volume, io, );
> + if (status != EFI_SUCCESS)
> + efi_printk(sys_table_arg, "Failed to open volume\n");
> +
> + *__fh = fh;
> + return status;
BTW., in the second failure branch is 'fh' guaranteed to be set by the EFI
call?
If not then we set *__fh to a potentially undefined value, from the kernels
stack?
(I realize that your refactoring just inherited this existing pattern, but it
caught my attention.)
Thanks,
Ingo
--
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 0/6] efi/x86 mixed mode cleanups
On Thu, Jul 12, 2018 at 02:21:48PM +0200, Ard Biesheuvel wrote:
> Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci
> routine.
>
> Patches #3 and #4 do the same for the UGA draw protocol discovery routines.
>
> Patch #6 helps unused code paths to be optimized away on configurations
> that don't need them (32-bit only and 64-bit only)
Thanks a lot for doing this Ard, I meant to deduplicate that code
further but didn't get around to it. FWIW I have a single unsubmitted
deduplication patch which has been rotting on my development branch for
more than a year now, I'm including it below in case it helps, needs a
careful review though.
-- >8 --
>From 7fa51faec20c74eea2bb3ba96ecab1bc3e18e5a8 Mon Sep 17 00:00:00 2001
From: Lukas Wunner
Date: Sun, 7 May 2017 14:42:51 +0200
Subject: [PATCH] efi: Deduplicate efi_open_volume()
There's one ARM, one x86_32 and one x86_64 version which can be folded
into a single shared version by masking their differences with the
efi_call_proto() macro introduced by commit 3552fdf29f01 ("efi: Allow
bitness-agnostic protocol calls").
To be able to dereference the device_handle attribute from the
efi_loaded_image_t table in an arch- and bitness-agnostic manner,
introduce the efi_table_attr() macro (which already exists for x86)
to arm and arm64.
No functional change intended.
Signed-off-by: Lukas Wunner
---
arch/arm/include/asm/efi.h | 3 ++
arch/arm64/include/asm/efi.h | 3 ++
arch/x86/boot/compressed/eboot.c | 61 --
drivers/firmware/efi/libstub/arm-stub.c| 25 ---
drivers/firmware/efi/libstub/efi-stub-helper.c | 29 +++-
drivers/firmware/efi/libstub/efistub.h | 3 --
include/linux/efi.h| 10 +
7 files changed, 43 insertions(+), 91 deletions(-)
diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 17f1f1a..38badaa 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -58,6 +58,9 @@ static inline void efi_set_pgd(struct mm_struct *mm)
#define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
#define efi_is_64bit() (false)
+#define efi_table_attr(table, attr, instance) \
+ ((table##_t *)instance)->attr
+
#define efi_call_proto(protocol, f, instance, ...) \
((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index c4cd508..5a21037 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -85,6 +85,9 @@ static inline unsigned long efi_get_max_initrd_addr(unsigned
long dram_base,
#define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
#define efi_is_64bit() (true)
+#define efi_table_attr(table, attr, instance) \
+ ((table##_t *)instance)->attr
+
#define efi_call_proto(protocol, f, instance, ...) \
((protocol##_t *)instance)->f(instance, ##__VA_ARGS__)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 9a5b6d3..2d2b599 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -41,67 +41,6 @@ __pure const struct efi_config *__efi_early(void)
BOOT_SERVICES(32);
BOOT_SERVICES(64);
-static inline efi_status_t __open_volume32(void *__image, void **__fh)
-{
- efi_file_io_interface_t *io;
- efi_loaded_image_32_t *image = __image;
- efi_file_handle_32_t *fh;
- efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
- efi_status_t status;
- void *handle = (void *)(unsigned long)image->device_handle;
- unsigned long func;
-
- status = efi_call_early(handle_protocol, handle,
- _proto, (void **));
- if (status != EFI_SUCCESS) {
- efi_printk(sys_table, "Failed to handle fs_proto\n");
- return status;
- }
-
- func = (unsigned long)io->open_volume;
- status = efi_early->call(func, io, );
- if (status != EFI_SUCCESS)
- efi_printk(sys_table, "Failed to open volume\n");
-
- *__fh = fh;
- return status;
-}
-
-static inline efi_status_t __open_volume64(void *__image, void **__fh)
-{
- efi_file_io_interface_t *io;
- efi_loaded_image_64_t *image = __image;
- efi_file_handle_64_t *fh;
- efi_guid_t fs_proto = EFI_FILE_SYSTEM_GUID;
- efi_status_t status;
- void *handle = (void *)(unsigned long)image->device_handle;
- unsigned long func;
-
- status = efi_call_early(handle_protocol, handle,
- _proto, (void **));
- if (status != EFI_SUCCESS) {
- efi_printk(sys_table, "Failed to handle fs_proto\n");
- return status;
- }
-
- func = (unsigned long)io->open_volume;
RE: [PATCH 0/6] efi/x86 mixed mode cleanups
> >> Patch #6 helps unused code paths to be optimized away on
> >> configurations that don't need them (32-bit only and 64-bit only)
> >
> > I have tested this patch set on qemu and I see mixed mode kernel not
> > booting.
> > My test setup is:
> > Running qemu-system-x86_64 with 32 bit OVMF and x86_64 kernel built with
> Mixed mode enabled.
> > I am using elilinux as bootloader.
> >
> > Upon further investigation, I found that the issue isn't related to this
> > patch set.
> > It's introduced with commit 2e6eb40ca5eb ("Fix incorrect invocation of
> > PciIO->Attributes") Reverting the change does help in booting mixed mode
> kernel.
> >
>
> Hello Sai,
>
> This is likely due to the fact that you are missing this patch
>
> https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=efi/urgen
> t=e296701800f30d260a66f8aa1971b5b1bc3d2f81
>
> Could you please double check? Thanks.
Thanks for updating me on this. I now see why my testing broke.
I think, the above patch is only in TIP tree and didn't yet make to Linus's
tree and is also not part of efi tree next branch (these were the trees I am
testing on).
Regards,
Sai
Re: [PATCH 0/6] efi/x86 mixed mode cleanups
On 13 July 2018 at 03:59, Prakhya, Sai Praneeth
wrote:
> Hi Ard,
>
>> This series contains some fixes and cleanups for mixed-mode UEFI on x86.
>>
>> Patch #1 adds the missing locking in the runtime service wrapper code for
>> mixed
>> mode. This was found by inspection rather than having been reported but could
>> be a candidate for -stable nonetheless.
>>
>> Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci
>> routine.
>>
>> Patches #3 and #4 do the same for the UGA draw protocol discovery routines.
>>
>> Patch #5 fixes a latent bug in the UGA draw code.
>>
>> Patch #6 helps unused code paths to be optimized away on configurations that
>> don't need them (32-bit only and 64-bit only)
>
> I have tested this patch set on qemu and I see mixed mode kernel not booting.
> My test setup is:
> Running qemu-system-x86_64 with 32 bit OVMF and x86_64 kernel built with
> Mixed mode enabled.
> I am using elilinux as bootloader.
>
> Upon further investigation, I found that the issue isn't related to this
> patch set.
> It's introduced with commit 2e6eb40ca5eb ("Fix incorrect invocation of
> PciIO->Attributes")
> Reverting the change does help in booting mixed mode kernel.
>
Hello Sai,
This is likely due to the fact that you are missing this patch
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=efi/urgent=e296701800f30d260a66f8aa1971b5b1bc3d2f81
Could you please double check? 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 0/6] efi/x86 mixed mode cleanups
Hi Ard,
> This series contains some fixes and cleanups for mixed-mode UEFI on x86.
>
> Patch #1 adds the missing locking in the runtime service wrapper code for
> mixed
> mode. This was found by inspection rather than having been reported but could
> be a candidate for -stable nonetheless.
>
> Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci
> routine.
>
> Patches #3 and #4 do the same for the UGA draw protocol discovery routines.
>
> Patch #5 fixes a latent bug in the UGA draw code.
>
> Patch #6 helps unused code paths to be optimized away on configurations that
> don't need them (32-bit only and 64-bit only)
I have tested this patch set on qemu and I see mixed mode kernel not booting.
My test setup is:
Running qemu-system-x86_64 with 32 bit OVMF and x86_64 kernel built with Mixed
mode enabled.
I am using elilinux as bootloader.
Upon further investigation, I found that the issue isn't related to this patch
set.
It's introduced with commit 2e6eb40ca5eb ("Fix incorrect invocation of
PciIO->Attributes")
Reverting the change does help in booting mixed mode kernel.
Regards,
Sai
--
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 0/6] efi/x86 mixed mode cleanups
On 12 July 2018 at 16:26, Hans de Goede wrote: > Hi, > > On 12-07-18 14:21, Ard Biesheuvel wrote: >> >> This series contains some fixes and cleanups for mixed-mode UEFI on x86. >> >> Patch #1 adds the missing locking in the runtime service wrapper code for >> mixed mode. This was found by inspection rather than having been reported >> but could be a candidate for -stable nonetheless. >> >> Patch #2 merges the remaining 32/64-bit specific parts of the >> setup_efi_pci >> routine. >> >> Patches #3 and #4 do the same for the UGA draw protocol discovery >> routines. >> >> Patch #5 fixes a latent bug in the UGA draw code. >> >> Patch #6 helps unused code paths to be optimized away on configurations >> that don't need them (32-bit only and 64-bit only) > > > I've given a kernel with these patches a quick spin running in mixed > mode on a Bay Trail based tablet: > > Tested-by: Hans de Goede > Thanks Hans, much appreciated. -- 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 0/6] efi/x86 mixed mode cleanups
Hi, On 12-07-18 14:21, Ard Biesheuvel wrote: This series contains some fixes and cleanups for mixed-mode UEFI on x86. Patch #1 adds the missing locking in the runtime service wrapper code for mixed mode. This was found by inspection rather than having been reported but could be a candidate for -stable nonetheless. Patch #2 merges the remaining 32/64-bit specific parts of the setup_efi_pci routine. Patches #3 and #4 do the same for the UGA draw protocol discovery routines. Patch #5 fixes a latent bug in the UGA draw code. Patch #6 helps unused code paths to be optimized away on configurations that don't need them (32-bit only and 64-bit only) I've given a kernel with these patches a quick spin running in mixed mode on a Bay Trail based tablet: Tested-by: Hans de Goede Regards, Hans -- 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
