From c7a9b6471c8ee6a2180fc5f2f7a1e284754bdfc5 Mon Sep 17 00:00:00 2001 From: "Eric W. Biederman" Date: Fri, 12 Nov 2021 15:12:38 -0600 Subject: [PATCH] signal/vm86_32: Remove pointless test in BUG_ON kernel test robot writes[1]: > > Greeting, > > FYI, we noticed the following commit (built with gcc-9): > > commit: 1a4d21a23c4ca7467726be7db9ae8077a62b2c62 ("signal/vm86_32: Replace open coded BUG_ON with an actual BUG_ON") > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git master > > in testcase: trinity > version: trinity-static-i386-x86_64-1c734c75-1_2020-01-06 > with following parameters: > > > [ 70.645554][ T3747] kernel BUG at arch/x86/kernel/vm86_32.c:109! > [ 70.646185][ T3747] invalid opcode: 0000 [#1] SMP > [ 70.646682][ T3747] CPU: 0 PID: 3747 Comm: trinity-c6 Not tainted 5.15.0-rc1-00009-g1a4d21a23c4c #1 > [ 70.647598][ T3747] EIP: save_v86_state (arch/x86/kernel/vm86_32.c:109 (discriminator 3)) > [ 70.648113][ T3747] Code: 89 c3 64 8b 35 60 b8 25 c2 83 ec 08 89 55 f0 8b 96 10 19 00 00 89 55 ec e8 c6 2d 0c 00 fb 8b 55 ec 85 d2 74 05 83 3a 00 75 02 <0f> 0b 8b 86 10 19 00 00 8b 4b 38 8b 78 48 31 cf 89 f8 8b 7a 4c 81 > [ 70.650136][ T3747] EAX: 00000001 EBX: f5f49fac ECX: 0000000b EDX: f610b600 > [ 70.650852][ T3747] ESI: f5f79cc0 EDI: f5f79cc0 EBP: f5f49f04 ESP: f5f49ef0 > [ 70.651593][ T3747] DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068 EFLAGS: 00010246 > [ 70.652413][ T3747] CR0: 80050033 CR2: 00004000 CR3: 35fc7000 CR4: 000406d0 > [ 70.653169][ T3747] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000 > [ 70.653897][ T3747] DR6: fffe0ff0 DR7: 00000400 > [ 70.654382][ T3747] Call Trace: > [ 70.654719][ T3747] arch_do_signal_or_restart (arch/x86/kernel/signal.c:792 arch/x86/kernel/signal.c:867) > [ 70.655288][ T3747] exit_to_user_mode_prepare (kernel/entry/common.c:174 kernel/entry/common.c:209) > [ 70.655854][ T3747] irqentry_exit_to_user_mode (kernel/entry/common.c:126 kernel/entry/common.c:317) > [ 70.656450][ T3747] irqentry_exit (kernel/entry/common.c:406) > [ 70.656897][ T3747] exc_page_fault (arch/x86/mm/fault.c:1535) > [ 70.657369][ T3747] ? sysvec_kvm_asyncpf_interrupt (arch/x86/mm/fault.c:1488) > [ 70.657989][ T3747] handle_exception (arch/x86/entry/entry_32.S:1085) vm86_32.c:109 is: "BUG_ON(!vm86 || !vm86->user_vm86)" When trying to understand the failure Brian Gerst pointed out[2] that the code does not need protection against vm86->user_vm86 being NULL. The copy_from_user code will already handles that case if the address is going to fault. Looking futher I realized that if we care about not allowing struct vm86plus_struct at address 0 it should be do_sys_vm86 (the system call) that does the filtering. Not way down deep when the emulation has completed in save_v86_state. So let's just remove the silly case of attempting to filter a userspace address with a BUG_ON. Existing userspace can't break and it won't make the kernel any more attackable as the userspace access helpers will handle it, if it isn't a good userspace pointer. I have run the reproducer the fuzzer gave me before I made this change and it reproduced, and after I made this change and I have not seen the reported failure. So it does looks like this fixes the reported issue. [1] https://lkml.kernel.org/r/20211112074030.GB19820@xsang-OptiPlex-9020 [2] https://lkml.kernel.org/r/CAMzpN2jkK5sAv-Kg_kVnCEyVySiqeTdUORcC=AdG1gV6r8nUew@mail.gmail.com Suggested-by: Brian Gerst Reported-by: kernel test robot Tested-by: "Eric W. Biederman" Signed-off-by: "Eric W. Biederman" --- arch/x86/kernel/vm86_32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c index f14f69d7aa3c..cce1c89cb7df 100644 --- a/arch/x86/kernel/vm86_32.c +++ b/arch/x86/kernel/vm86_32.c @@ -106,7 +106,7 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval) */ local_irq_enable(); - BUG_ON(!vm86 || !vm86->user_vm86); + BUG_ON(!vm86); set_flags(regs->pt.flags, VEFLAGS, X86_EFLAGS_VIF | vm86->veflags_mask); user = vm86->user_vm86;