From 3463203d109608f24b75b24c90db260c7c192064 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Thu, 22 Dec 2022 18:38:20 +0000 Subject: firmware: arm_scmi: Harden shared memory access in fetch_response [ Upstream commit ad78b81a1077f7d956952cd8bdfe1e61504e3eb8 ] A misbheaving SCMI platform firmware could reply with out-of-spec messages, shorter than the mimimum size comprising a header and a status field. Harden shmem_fetch_response to properly truncate such a bad messages. Fixes: 5c8a47a5a91d ("firmware: arm_scmi: Make scmi core independent of the transport type") Signed-off-by: Cristian Marussi Link: https://lore.kernel.org/r/20221222183823.518856-3-cristian.marussi@arm.com Signed-off-by: Sudeep Holla Signed-off-by: Sasha Levin --- drivers/firmware/arm_scmi/shmem.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c index 1dfe534b8518..135f8718000f 100644 --- a/drivers/firmware/arm_scmi/shmem.c +++ b/drivers/firmware/arm_scmi/shmem.c @@ -81,10 +81,11 @@ u32 shmem_read_header(struct scmi_shared_mem __iomem *shmem) void shmem_fetch_response(struct scmi_shared_mem __iomem *shmem, struct scmi_xfer *xfer) { + size_t len = ioread32(&shmem->length); + xfer->hdr.status = ioread32(shmem->msg_payload); /* Skip the length of header and status in shmem area i.e 8 bytes */ - xfer->rx.len = min_t(size_t, xfer->rx.len, - ioread32(&shmem->length) - 8); + xfer->rx.len = min_t(size_t, xfer->rx.len, len > 8 ? len - 8 : 0); /* Take a copy to the rx buffer.. */ memcpy_fromio(xfer->rx.buf, shmem->msg_payload + 4, xfer->rx.len); -- cgit 1.4.1 From d807aff1910c81fa20c78edde7523a01be9669f1 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Thu, 22 Dec 2022 18:38:21 +0000 Subject: firmware: arm_scmi: Harden shared memory access in fetch_notification [ Upstream commit 9bae076cd4e3e3c3dc185cae829d80b2dddec86e ] A misbheaving SCMI platform firmware could reply with out-of-spec notifications, shorter than the mimimum size comprising a header. Fixes: d5141f37c42e ("firmware: arm_scmi: Add notifications support in transport layer") Signed-off-by: Cristian Marussi Link: https://lore.kernel.org/r/20221222183823.518856-4-cristian.marussi@arm.com Signed-off-by: Sudeep Holla Signed-off-by: Sasha Levin --- drivers/firmware/arm_scmi/shmem.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/arm_scmi/shmem.c b/drivers/firmware/arm_scmi/shmem.c index 135f8718000f..87b4f4d35f06 100644 --- a/drivers/firmware/arm_scmi/shmem.c +++ b/drivers/firmware/arm_scmi/shmem.c @@ -94,8 +94,10 @@ void shmem_fetch_response(struct scmi_shared_mem __iomem *shmem, void shmem_fetch_notification(struct scmi_shared_mem __iomem *shmem, size_t max_len, struct scmi_xfer *xfer) { + size_t len = ioread32(&shmem->length); + /* Skip only the length of header in shmem area i.e 4 bytes */ - xfer->rx.len = min_t(size_t, max_len, ioread32(&shmem->length) - 4); + xfer->rx.len = min_t(size_t, max_len, len > 4 ? len - 4 : 0); /* Take a copy to the rx buffer.. */ memcpy_fromio(xfer->rx.buf, shmem->msg_payload, xfer->rx.len); -- cgit 1.4.1 From 5362263b0c2f17775bf9507f4a3daa2fad3f8407 Mon Sep 17 00:00:00 2001 From: Cristian Marussi Date: Thu, 22 Dec 2022 18:38:23 +0000 Subject: firmware: arm_scmi: Fix virtio channels cleanup on shutdown [ Upstream commit e325285de2cd82fbdcc4df8898e4c6a597674816 ] When unloading the SCMI core stack module, configured to use the virtio SCMI transport, LOCKDEP reports the splat down below about unsafe locks dependencies. In order to avoid this possible unsafe locking scenario call upfront virtio_break_device() before getting hold of vioch->lock. ===================================================== WARNING: HARDIRQ-safe -> HARDIRQ-unsafe lock order detected 6.1.0-00067-g6b934395ba07-dirty #4 Not tainted ----------------------------------------------------- rmmod/307 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire: ffff000080c510e0 (&dev->vqs_list_lock){+.+.}-{3:3}, at: virtio_break_device+0x28/0x68 and this task is already holding: ffff00008288ada0 (&channels[i].lock){-.-.}-{3:3}, at: virtio_chan_free+0x60/0x168 [scmi_module] which would create a new lock dependency: (&channels[i].lock){-.-.}-{3:3} -> (&dev->vqs_list_lock){+.+.}-{3:3} but this new dependency connects a HARDIRQ-irq-safe lock: (&channels[i].lock){-.-.}-{3:3} ... which became HARDIRQ-irq-safe at: lock_acquire+0x128/0x398 _raw_spin_lock_irqsave+0x78/0x140 scmi_vio_complete_cb+0xb4/0x3b8 [scmi_module] vring_interrupt+0x84/0x120 vm_interrupt+0x94/0xe8 __handle_irq_event_percpu+0xb4/0x3d8 handle_irq_event_percpu+0x20/0x68 handle_irq_event+0x50/0xb0 handle_fasteoi_irq+0xac/0x138 generic_handle_domain_irq+0x34/0x50 gic_handle_irq+0xa0/0xd8 call_on_irq_stack+0x2c/0x54 do_interrupt_handler+0x8c/0x90 el1_interrupt+0x40/0x78 el1h_64_irq_handler+0x18/0x28 el1h_64_irq+0x64/0x68 _raw_write_unlock_irq+0x48/0x80 ep_start_scan+0xf0/0x128 do_epoll_wait+0x390/0x858 do_compat_epoll_pwait.part.34+0x1c/0xb8 __arm64_sys_epoll_pwait+0x80/0xd0 invoke_syscall+0x4c/0x110 el0_svc_common.constprop.3+0x98/0x120 do_el0_svc+0x34/0xd0 el0_svc+0x40/0x98 el0t_64_sync_handler+0x98/0xc0 el0t_64_sync+0x170/0x174 to a HARDIRQ-irq-unsafe lock: (&dev->vqs_list_lock){+.+.}-{3:3} ... which became HARDIRQ-irq-unsafe at: ... lock_acquire+0x128/0x398 _raw_spin_lock+0x58/0x70 __vring_new_virtqueue+0x130/0x1c0 vring_create_virtqueue+0xc4/0x2b8 vm_find_vqs+0x20c/0x430 init_vq+0x308/0x390 virtblk_probe+0x114/0x9b0 virtio_dev_probe+0x1a4/0x248 really_probe+0xc8/0x3a8 __driver_probe_device+0x84/0x190 driver_probe_device+0x44/0x110 __driver_attach+0x104/0x1e8 bus_for_each_dev+0x7c/0xd0 driver_attach+0x2c/0x38 bus_add_driver+0x1e4/0x258 driver_register+0x6c/0x128 register_virtio_driver+0x2c/0x48 virtio_blk_init+0x70/0xac do_one_initcall+0x84/0x420 kernel_init_freeable+0x2d0/0x340 kernel_init+0x2c/0x138 ret_from_fork+0x10/0x20 other info that might help us debug this: Possible interrupt unsafe locking scenario: CPU0 CPU1 ---- ---- lock(&dev->vqs_list_lock); local_irq_disable(); lock(&channels[i].lock); lock(&dev->vqs_list_lock); lock(&channels[i].lock); *** DEADLOCK *** ================ Fixes: 42e90eb53bf3f ("firmware: arm_scmi: Add a virtio channel refcount") Signed-off-by: Cristian Marussi Link: https://lore.kernel.org/r/20221222183823.518856-6-cristian.marussi@arm.com Signed-off-by: Sudeep Holla Signed-off-by: Sasha Levin --- drivers/firmware/arm_scmi/virtio.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/arm_scmi/virtio.c b/drivers/firmware/arm_scmi/virtio.c index 33c9b81a55cd..1db975c08896 100644 --- a/drivers/firmware/arm_scmi/virtio.c +++ b/drivers/firmware/arm_scmi/virtio.c @@ -160,7 +160,6 @@ static void scmi_vio_channel_cleanup_sync(struct scmi_vio_channel *vioch) } vioch->shutdown_done = &vioch_shutdown_done; - virtio_break_device(vioch->vqueue->vdev); if (!vioch->is_rx && vioch->deferred_tx_wq) /* Cannot be kicked anymore after this...*/ vioch->deferred_tx_wq = NULL; @@ -482,6 +481,12 @@ static int virtio_chan_free(int id, void *p, void *data) struct scmi_chan_info *cinfo = p; struct scmi_vio_channel *vioch = cinfo->transport_info; + /* + * Break device to inhibit further traffic flowing while shutting down + * the channels: doing it later holding vioch->lock creates unsafe + * locking dependency chains as reported by LOCKDEP. + */ + virtio_break_device(vioch->vqueue->vdev); scmi_vio_channel_cleanup_sync(vioch); scmi_free_channel(cinfo, data, id); -- cgit 1.4.1 From 5f366b36a83bf71df0ed35bf5758edf473eca1aa Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Thu, 12 Jan 2023 15:03:16 -0800 Subject: firmware: coreboot: Check size of table entry and use flex-array [ Upstream commit 3b293487b8752cc42c1cbf8a0447bc6076c075fa ] The memcpy() of the data following a coreboot_table_entry couldn't be evaluated by the compiler under CONFIG_FORTIFY_SOURCE. To make it easier to reason about, add an explicit flexible array member to struct coreboot_device so the entire entry can be copied at once. Additionally, validate the sizes before copying. Avoids this run-time false positive warning: memcpy: detected field-spanning write (size 168) of single field "&device->entry" at drivers/firmware/google/coreboot_table.c:103 (size 8) Reported-by: Paul Menzel Link: https://lore.kernel.org/all/03ae2704-8c30-f9f0-215b-7cdf4ad35a9a@molgen.mpg.de/ Cc: Jack Rosenthal Cc: Guenter Roeck Cc: Julius Werner Cc: Brian Norris Cc: Stephen Boyd Cc: Greg Kroah-Hartman Signed-off-by: Kees Cook Reviewed-by: Julius Werner Reviewed-by: Guenter Roeck Link: https://lore.kernel.org/r/20230107031406.gonna.761-kees@kernel.org Reviewed-by: Stephen Boyd Reviewed-by: Jack Rosenthal Link: https://lore.kernel.org/r/20230112230312.give.446-kees@kernel.org Signed-off-by: Sasha Levin --- drivers/firmware/google/coreboot_table.c | 9 +++++++-- drivers/firmware/google/coreboot_table.h | 1 + 2 files changed, 8 insertions(+), 2 deletions(-) (limited to 'drivers/firmware') diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c index 9ca21feb9d45..f3694d347801 100644 --- a/drivers/firmware/google/coreboot_table.c +++ b/drivers/firmware/google/coreboot_table.c @@ -93,7 +93,12 @@ static int coreboot_table_populate(struct device *dev, void *ptr) for (i = 0; i < header->table_entries; i++) { entry = ptr_entry; - device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL); + if (entry->size < sizeof(*entry)) { + dev_warn(dev, "coreboot table entry too small!\n"); + return -EINVAL; + } + + device = kzalloc(sizeof(device->dev) + entry->size, GFP_KERNEL); if (!device) return -ENOMEM; @@ -101,7 +106,7 @@ static int coreboot_table_populate(struct device *dev, void *ptr) device->dev.parent = dev; device->dev.bus = &coreboot_bus_type; device->dev.release = coreboot_device_release; - memcpy(&device->entry, ptr_entry, entry->size); + memcpy(device->raw, ptr_entry, entry->size); ret = device_register(&device->dev); if (ret) { diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h index beb778674acd..4a89277b99a3 100644 --- a/drivers/firmware/google/coreboot_table.h +++ b/drivers/firmware/google/coreboot_table.h @@ -66,6 +66,7 @@ struct coreboot_device { struct coreboot_table_entry entry; struct lb_cbmem_ref cbmem_ref; struct lb_framebuffer framebuffer; + DECLARE_FLEX_ARRAY(u8, raw); }; }; -- cgit 1.4.1 From 119a34527ed85403c1926b3b6f880dd6a75c4c29 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Fri, 28 Oct 2022 16:39:14 +0200 Subject: arm64: efi: Recover from synchronous exceptions occurring in firmware [ Upstream commit e8dfdf3162eb549d064b8c10b1564f7e8ee82591 ] Unlike x86, which has machinery to deal with page faults that occur during the execution of EFI runtime services, arm64 has nothing like that, and a synchronous exception raised by firmware code brings down the whole system. With more EFI based systems appearing that were not built to run Linux (such as the Windows-on-ARM laptops based on Qualcomm SOCs), as well as the introduction of PRM (platform specific firmware routines that are callable just like EFI runtime services), we are more likely to run into issues of this sort, and it is much more likely that we can identify and work around such issues if they don't bring down the system entirely. Since we already use a EFI runtime services call wrapper in assembler, we can quite easily add some code that captures the execution state at the point where the call is made, allowing us to revert to this state and proceed execution if the call triggered a synchronous exception. Given that the kernel and the firmware don't share any data structures that could end up in an indeterminate state, we can happily continue running, as long as we mark the EFI runtime services as unavailable from that point on. Signed-off-by: Ard Biesheuvel Acked-by: Catalin Marinas Stable-dep-of: 8a9a1a18731e ("arm64: efi: Avoid workqueue to check whether EFI runtime is live") Signed-off-by: Sasha Levin --- arch/arm64/include/asm/efi.h | 8 ++++++++ arch/arm64/kernel/efi-rt-wrapper.S | 32 +++++++++++++++++++++++++++----- arch/arm64/kernel/efi.c | 22 ++++++++++++++++++++++ arch/arm64/mm/fault.c | 4 ++++ drivers/firmware/efi/runtime-wrappers.c | 1 + 5 files changed, 62 insertions(+), 5 deletions(-) (limited to 'drivers/firmware') diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index b9f3165075c9..0edaf8e385b8 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -14,8 +14,16 @@ #ifdef CONFIG_EFI extern void efi_init(void); + +bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg); #else #define efi_init() + +static inline +bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg) +{ + return false; +} #endif int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md); diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S index 2d3c4b02393e..d872d18101d8 100644 --- a/arch/arm64/kernel/efi-rt-wrapper.S +++ b/arch/arm64/kernel/efi-rt-wrapper.S @@ -7,7 +7,7 @@ #include SYM_FUNC_START(__efi_rt_asm_wrapper) - stp x29, x30, [sp, #-32]! + stp x29, x30, [sp, #-112]! mov x29, sp /* @@ -17,11 +17,21 @@ SYM_FUNC_START(__efi_rt_asm_wrapper) */ stp x1, x18, [sp, #16] + /* + * Preserve all callee saved registers and preserve the stack pointer + * value at the base of the EFI runtime stack so we can recover from + * synchronous exceptions occurring while executing the firmware + * routines. + */ + stp x19, x20, [sp, #32] + stp x21, x22, [sp, #48] + stp x23, x24, [sp, #64] + stp x25, x26, [sp, #80] + stp x27, x28, [sp, #96] + ldr_l x16, efi_rt_stack_top mov sp, x16 -#ifdef CONFIG_SHADOW_CALL_STACK - str x18, [sp, #-16]! -#endif + stp x18, x29, [sp, #-16]! /* * We are lucky enough that no EFI runtime services take more than @@ -39,7 +49,7 @@ SYM_FUNC_START(__efi_rt_asm_wrapper) mov sp, x29 ldp x1, x2, [sp, #16] cmp x2, x18 - ldp x29, x30, [sp], #32 + ldp x29, x30, [sp], #112 b.ne 0f ret 0: @@ -57,3 +67,15 @@ SYM_FUNC_START(__efi_rt_asm_wrapper) b efi_handle_corrupted_x18 // tail call SYM_FUNC_END(__efi_rt_asm_wrapper) + +SYM_CODE_START(__efi_rt_asm_recover) + mov sp, x30 + + ldp x19, x20, [sp, #32] + ldp x21, x22, [sp, #48] + ldp x23, x24, [sp, #64] + ldp x25, x26, [sp, #80] + ldp x27, x28, [sp, #96] + ldp x29, x30, [sp], #112 + ret +SYM_CODE_END(__efi_rt_asm_recover) diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index 386bd81ca12b..fab05de2e12d 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -149,6 +149,28 @@ DEFINE_SPINLOCK(efi_rt_lock); asmlinkage u64 *efi_rt_stack_top __ro_after_init; +asmlinkage efi_status_t __efi_rt_asm_recover(void); + +bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg) +{ + /* Check whether the exception occurred while running the firmware */ + if (current_work() != &efi_rts_work.work || regs->pc >= TASK_SIZE_64) + return false; + + pr_err(FW_BUG "Unable to handle %s in EFI runtime service\n", msg); + add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK); + clear_bit(EFI_RUNTIME_SERVICES, &efi.flags); + + regs->regs[0] = EFI_ABORTED; + regs->regs[30] = efi_rt_stack_top[-1]; + regs->pc = (u64)__efi_rt_asm_recover; + + if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK)) + regs->regs[18] = efi_rt_stack_top[-2]; + + return true; +} + /* EFI requires 8 KiB of stack space for runtime services */ static_assert(THREAD_SIZE >= SZ_8K); diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index 74f76514a48d..3eb2825d08cf 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -397,6 +398,9 @@ static void __do_kernel_fault(unsigned long addr, unsigned long esr, msg = "paging request"; } + if (efi_runtime_fixup_exception(regs, msg)) + return; + die_kernel_fault(msg, addr, esr, regs); } diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c index 60075e0e4943..1fba4e09cdcf 100644 --- a/drivers/firmware/efi/runtime-wrappers.c +++ b/drivers/firmware/efi/runtime-wrappers.c @@ -84,6 +84,7 @@ struct efi_runtime_work efi_rts_work; else \ pr_err("Failed to queue work to efi_rts_wq.\n"); \ \ + WARN_ON_ONCE(efi_rts_work.status == EFI_ABORTED); \ exit: \ efi_rts_work.efi_rts_id = EFI_NONE; \ efi_rts_work.status; \ -- cgit 1.4.1