• Eric W. Biederman's avatar
    signal/vm86_32: Remove pointless test in BUG_ON · c7a9b647
    Eric W. Biederman authored
    kernel test robot <oliver.sang@intel.com> writes[1]:
    >
    > Greeting,
    >
    > FYI, we noticed the following commit (built with gcc-9):
    >
    > commit: 1a4d21a2 ("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-g1a4d21a2 #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.comSuggested-by: default avatarBrian Gerst <brgerst@gmail.com>
    Reported-by: default avatarkernel test robot <oliver.sang@intel.com>
    Tested-by: default avatar"Eric W. Biederman" <ebiederm@xmission.com>
    Signed-off-by: default avatar"Eric W. Biederman" <ebiederm@xmission.com>
    c7a9b647
vm86_32.c 22 KB