From 923c95c84d7081d7be9503bf5b276dd93bd17036 Mon Sep 17 00:00:00 2001 From: Fabiano Rosas Date: Mon, 16 Aug 2021 21:12:24 -0400 Subject: [PATCH] powerpc/mm: Update pte atomically in change_page_attr Current code first invalidates the PTE and then sets a new PTE with the desired protection bits. This leaves a window in between when the PTE is invalid and translation attempts will fault. Since this is inside the kernel, the page fault handler raises a BUG. This issue was observed during the probe of virtio console: kernel tried to execute exec-protected page (c008000000692da8) - exploit attempt? (uid: 0) BUG: Unable to handle kernel instruction fetch Faulting instruction address: 0xc008000000692da8 Oops: Kernel access of bad area, sig: 11 [#1] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries Modules linked in: virtio_console virtio_blk(-) ... CPU: 7 PID: 169 Comm: kworker/7:1 Not tainted 5.14.0-rc5-dirty #54 random: crng init done Workqueue: events control_work_handler [virtio_console] random: 7 urandom warning(s) missed due to ratelimiting NIP: c008000000692da8 LR: c008000000692da8 CTR: c000000000213df0 REGS: c00000000ad4f770 TRAP: 0400 Not tainted (5.14.0-rc5-dirty) MSR: 800000004280b033 CR: 22002284 XER: 00000000 CFAR: c000000000213e4c IRQMASK: 1 GPR00: c008000000692da8 c00000000ad4fa10 c0000000027d1600 c00000000deb4320 GPR04: 0000000000000001 0000000000000000 0000000000000000 0000000000000000 GPR08: 00000000000000ff c00000000deb4320 0000000000000001 c0080000006963a0 GPR12: c000000000213df0 c0000007ffff4200 c0000000001c7888 c000000007e70440 GPR16: 0000000000000000 0000000000000000 c00000000c700000 c000000014c30300 GPR20: 0000000000000001 0000000000000000 0000000000000000 c000000014c30540 GPR24: c0080000006a0288 0000000000000000 0000000000000000 c00000000deb4320 GPR28: c000000014c303c0 0000000000000028 0000000000000058 c00000000b3225e0 NIP [c008000000692da8] fill_queue+0x140/0x230 [virtio_console] LR [c008000000692da8] fill_queue+0x140/0x230 [virtio_console] Call Trace: [c00000000ad4fa10] [c008000000692d58] fill_queue+0xf0/0x230 [virtio_console] (unreliable) [c00000000ad4faa0] [c008000000693228] add_port.isra.0+0x1a0/0x4b0 [virtio_console] [c00000000ad4fb70] [c008000000695494] control_work_handler+0x46c/0x654 [virtio_console] [c00000000ad4fc70] [c0000000001bac00] process_one_work+0x2a0/0x570 [c00000000ad4fd10] [c0000000001baf78] worker_thread+0xa8/0x660 [c00000000ad4fda0] [c0000000001c79fc] kthread+0x17c/0x190 [c00000000ad4fe10] [c00000000000cfd4] ret_from_kernel_thread+0x5c/0x64 Instruction dump: 2c2a0000 3bbd0001 7fbd07b4 4182ff4c b32d118a 7c0004ac 4bffff40 60000000 60000000 60420000 7f63db78 480035fd 38600000 48002ee9 e8410018 ---[ end trace 621a3f8c51128d94 ]--- The race happens because the virtio_console module's init sets up a work handler that can be made to run while module.c:do_init_module is still doing post-init tasks, one of which is setting the module code to read-only with module_enable_ro. The fact that we set the page to read-only is not relevant, only that we invalidate the PTE while doing so. This patch alters the way we update the PTE so that there is no window for the bug to happen. Signed-off-by: Fabiano Rosas --- arch/powerpc/mm/pageattr.c | 25 +++---------------------- 1 file changed, 3 insertions(+), 22 deletions(-) diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c index 0876216ceee671..a9977087630262 100644 --- a/arch/powerpc/mm/pageattr.c +++ b/arch/powerpc/mm/pageattr.c @@ -15,30 +15,13 @@ #include -/* - * Updates the attributes of a page in three steps: - * - * 1. invalidate the page table entry - * 2. flush the TLB - * 3. install the new entry with the updated attributes - * - * Invalidating the pte means there are situations where this will not work - * when in theory it should. - * For example: - * - removing write from page whilst it is being executed - * - setting a page read-only whilst it is being read by another CPU - * - */ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data) { long action = (long)data; pte_t pte; spin_lock(&init_mm.page_table_lock); - - /* invalidate the PTE so it's safe to modify */ - pte = ptep_get_and_clear(&init_mm, addr, ptep); - flush_tlb_kernel_range(addr, addr + PAGE_SIZE); + pte = *ptep; /* modify the PTE bits as desired, then apply */ switch (action) { @@ -59,11 +42,9 @@ static int change_page_attr(pte_t *ptep, unsigned long addr, void *data) break; } - set_pte_at(&init_mm, addr, ptep, pte); + pte_update(&init_mm, addr, ptep, ~0UL, pte_val(pte), 0); + flush_tlb_kernel_range(addr, addr + PAGE_SIZE); - /* See ptesync comment in radix__set_pte_at() */ - if (radix_enabled()) - asm volatile("ptesync": : :"memory"); spin_unlock(&init_mm.page_table_lock); return 0;