| Message ID | 20240604155625.2197275-2-ardb+git@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efi: Add missing __nocfi annotations to runtime wrappers | expand |
On Tue, Jun 4, 2024 at 5:56 PM Ard Biesheuvel <ardb+git@google.com> wrote: > From: Ard Biesheuvel <ardb@kernel.org> > > The EFI runtime wrappers are a sandbox for calling into EFI runtime > services, which are invoked using indirect calls. When running with kCFI > enabled, the compiler will require the target of any indirect call to be > type annotated. > > Given that the EFI runtime services prototypes and calling convention > are governed by the EFI spec, not the Linux kernel, adding such type > annotations for firmware routines is infeasible, and so the compiler > must be informed that prototype validation should be omitted. > > Add the __nocfi annotation at the appropriate places in the EFI runtime > wrapper code to achieve this. > > Note that this currently only affects 32-bit ARM, given that other > architectures that support both kCFI and EFI use an asm wrapper to call > EFI runtime services, and this hides the indirect call from the > compiler. > > Cc: Kees Cook <keescook@chromium.org> > Cc: Sami Tolvanen <samitolvanen@google.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Cc: Nathan Chancellor <nathan@kernel.org> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Thanks for looking into this Ard! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Maybe tag on: Fixes: 1a4fec49efe5 ("ARM: 9392/2: Support CLANG CFI") So it goes into the v6.10-rc:s. Yours, Linus Walleij
On Tue, 4 Jun 2024 at 23:05, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Jun 4, 2024 at 5:56 PM Ard Biesheuvel <ardb+git@google.com> wrote: > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > The EFI runtime wrappers are a sandbox for calling into EFI runtime > > services, which are invoked using indirect calls. When running with kCFI > > enabled, the compiler will require the target of any indirect call to be > > type annotated. > > > > Given that the EFI runtime services prototypes and calling convention > > are governed by the EFI spec, not the Linux kernel, adding such type > > annotations for firmware routines is infeasible, and so the compiler > > must be informed that prototype validation should be omitted. > > > > Add the __nocfi annotation at the appropriate places in the EFI runtime > > wrapper code to achieve this. > > > > Note that this currently only affects 32-bit ARM, given that other > > architectures that support both kCFI and EFI use an asm wrapper to call > > EFI runtime services, and this hides the indirect call from the > > compiler. > > > > Cc: Kees Cook <keescook@chromium.org> > > Cc: Sami Tolvanen <samitolvanen@google.com> > > Cc: Linus Walleij <linus.walleij@linaro.org> > > Cc: Nathan Chancellor <nathan@kernel.org> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > Thanks for looking into this Ard! > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Maybe tag on: > Fixes: 1a4fec49efe5 ("ARM: 9392/2: Support CLANG CFI") > > So it goes into the v6.10-rc:s. > Thanks, I've added these and pushed the result to efi/urgent.
On Tue, Jun 04, 2024 at 11:26:51PM +0200, Ard Biesheuvel wrote: > On Tue, 4 Jun 2024 at 23:05, Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Tue, Jun 4, 2024 at 5:56 PM Ard Biesheuvel <ardb+git@google.com> wrote: > > > > > From: Ard Biesheuvel <ardb@kernel.org> > > > > > > The EFI runtime wrappers are a sandbox for calling into EFI runtime > > > services, which are invoked using indirect calls. When running with kCFI > > > enabled, the compiler will require the target of any indirect call to be > > > type annotated. > > > > > > Given that the EFI runtime services prototypes and calling convention > > > are governed by the EFI spec, not the Linux kernel, adding such type > > > annotations for firmware routines is infeasible, and so the compiler > > > must be informed that prototype validation should be omitted. > > > > > > Add the __nocfi annotation at the appropriate places in the EFI runtime > > > wrapper code to achieve this. > > > > > > Note that this currently only affects 32-bit ARM, given that other > > > architectures that support both kCFI and EFI use an asm wrapper to call > > > EFI runtime services, and this hides the indirect call from the > > > compiler. > > > > > > Cc: Kees Cook <keescook@chromium.org> > > > Cc: Sami Tolvanen <samitolvanen@google.com> > > > Cc: Linus Walleij <linus.walleij@linaro.org> > > > Cc: Nathan Chancellor <nathan@kernel.org> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > > Thanks for looking into this Ard! > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > > > Maybe tag on: > > Fixes: 1a4fec49efe5 ("ARM: 9392/2: Support CLANG CFI") > > > > So it goes into the v6.10-rc:s. > > > > Thanks, I've added these and pushed the result to efi/urgent. You don't need to rebase to include it but just for the record, I tested it as well and it resolves the crash I saw when booting under EFI in QEMU with CONFIG_CFI_CLANG=y. Tested-by: Nathan Chancellor <nathan@kernel.org> Cheers, Nathan
On Wed, Jun 5, 2024 at 8:06 AM Nathan Chancellor <nathan@kernel.org> wrote: > You don't need to rebase to include it but just for the record, I tested > it as well and it resolves the crash I saw when booting under EFI in > QEMU with CONFIG_CFI_CLANG=y. > > Tested-by: Nathan Chancellor <nathan@kernel.org> Thanks for testing the EFI boot Nathan! Now my only remaining worry is BPF trampolines, I will try to read up on how to run BPF on ARM32 unless someone beats me to it. Yours, Linus Walleij
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 5d56bc40a79d..708b777857d3 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -213,7 +213,7 @@ extern struct semaphore __efi_uv_runtime_lock __alias(efi_runtime_lock); * Calls the appropriate efi_runtime_service() with the appropriate * arguments. */ -static void efi_call_rts(struct work_struct *work) +static void __nocfi efi_call_rts(struct work_struct *work) { const union efi_rts_args *args = efi_rts_work.args; efi_status_t status = EFI_NOT_FOUND; @@ -435,7 +435,7 @@ static efi_status_t virt_efi_set_variable(efi_char16_t *name, return status; } -static efi_status_t +static efi_status_t __nocfi virt_efi_set_variable_nb(efi_char16_t *name, efi_guid_t *vendor, u32 attr, unsigned long data_size, void *data) { @@ -469,7 +469,7 @@ static efi_status_t virt_efi_query_variable_info(u32 attr, return status; } -static efi_status_t +static efi_status_t __nocfi virt_efi_query_variable_info_nb(u32 attr, u64 *storage_space, u64 *remaining_space, u64 *max_variable_size) { @@ -499,10 +499,9 @@ static efi_status_t virt_efi_get_next_high_mono_count(u32 *count) return status; } -static void virt_efi_reset_system(int reset_type, - efi_status_t status, - unsigned long data_size, - efi_char16_t *data) +static void __nocfi +virt_efi_reset_system(int reset_type, efi_status_t status, + unsigned long data_size, efi_char16_t *data) { if (down_trylock(&efi_runtime_lock)) { pr_warn("failed to invoke the reset_system() runtime service:\n"