lkml.org 
[lkml]   [2021]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    Subject[GIT PULL ] signal/vm86_32: Remove pointless test in BUG_ON

    Linus,

    Please pull the exit-cleanups-for-v5.16 branch from the git tree:

    git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git exit-cleanups-for-v5.16

    HEAD: c7a9b6471c8ee6a2180fc5f2f7a1e284754bdfc5 signal/vm86_32: Remove pointless test in BUG_ON


    This branch has only one unpulled change. Just the removal of an
    unnecessary test from a BUG_ON. Which in my running of the fuzzer
    locally fixes the issue.

    kernel test robot <oliver.sang@intel.com> 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 <brgerst@gmail.com>
    Reported-by: kernel test robot <oliver.sang@intel.com>
    Tested-by: "Eric W. Biederman" <ebiederm@xmission.com>
    Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
    ---
    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;
    --
    2.20.1
    \
     
     \ /
      Last update: 2021-11-12 22:38    [W:4.624 / U:0.252 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site