| Message ID | 20181220174514.24953-1-paul.burton@mips.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | adcc81f148d733b7e8e641300c5590a2cdc13bf3 |
| Headers | show |
| Series | MIPS: math-emu: Write-protect delay slot emulation pages | expand |
Hi Sasha, On Thu, Dec 20, 2018 at 07:26:15PM +0000, Sasha Levin wrote: > Hi, > > [This is an automated email] > > This commit has been processed because it contains a "Fixes:" tag, > fixing commit: 432c6bacbd0c MIPS: Use per-mm page to execute branch delay slot instructions. > > The bot has tested the following trees: v4.19.10, v4.14.89, v4.9.146, Neat! I like the idea of this automation :) > v4.19.10: Build OK! > v4.14.89: Build OK! > v4.9.146: Failed to apply! Possible dependencies: > 05ce77249d50 ("userfaultfd: non-cooperative: add madvise() event for MADV_DONTNEED request") > 163e11bc4f6e ("userfaultfd: hugetlbfs: UFFD_FEATURE_MISSING_HUGETLBFS") > 67dece7d4c58 ("x86/vdso: Set vDSO pointer only after success") > 72f87654c696 ("userfaultfd: non-cooperative: add mremap() event") > 893e26e61d04 ("userfaultfd: non-cooperative: Add fork() event") > 897ab3e0c49e ("userfaultfd: non-cooperative: add event for memory unmaps") > 9cd75c3cd4c3 ("userfaultfd: non-cooperative: add ability to report non-PF events from uffd descriptor") > d811914d8757 ("userfaultfd: non-cooperative: rename *EVENT_MADVDONTNEED to *EVENT_REMOVE") This list includes the correct soft dependency - commit 897ab3e0c49e ("userfaultfd: non-cooperative: add event for memory unmaps") which added an extra argument to mmap_region(). > How should we proceed with this patch? The backport to v4.9 should simply drop the last argument (NULL) in the call to mmap_region(). Is there some way I can indicate this sort of thing in future patches so that the automation can spot that I already know it won't apply cleanly to a particular range of kernel versions? Or even better, that I could indicate what needs to be changed when backporting to those kernel versions? Thanks, Paul
On Fri, Dec 21, 2018 at 09:16:37PM +0000, Paul Burton wrote: >Hi Sasha, > >On Thu, Dec 20, 2018 at 07:26:15PM +0000, Sasha Levin wrote: >> Hi, >> >> [This is an automated email] >> >> This commit has been processed because it contains a "Fixes:" tag, >> fixing commit: 432c6bacbd0c MIPS: Use per-mm page to execute branch delay slot instructions. >> >> The bot has tested the following trees: v4.19.10, v4.14.89, v4.9.146, > >Neat! I like the idea of this automation :) Thank you! :) >> v4.19.10: Build OK! >> v4.14.89: Build OK! >> v4.9.146: Failed to apply! Possible dependencies: >> 05ce77249d50 ("userfaultfd: non-cooperative: add madvise() event for MADV_DONTNEED request") >> 163e11bc4f6e ("userfaultfd: hugetlbfs: UFFD_FEATURE_MISSING_HUGETLBFS") >> 67dece7d4c58 ("x86/vdso: Set vDSO pointer only after success") >> 72f87654c696 ("userfaultfd: non-cooperative: add mremap() event") >> 893e26e61d04 ("userfaultfd: non-cooperative: Add fork() event") >> 897ab3e0c49e ("userfaultfd: non-cooperative: add event for memory unmaps") >> 9cd75c3cd4c3 ("userfaultfd: non-cooperative: add ability to report non-PF events from uffd descriptor") >> d811914d8757 ("userfaultfd: non-cooperative: rename *EVENT_MADVDONTNEED to *EVENT_REMOVE") > >This list includes the correct soft dependency - commit 897ab3e0c49e >("userfaultfd: non-cooperative: add event for memory unmaps") which >added an extra argument to mmap_region(). > >> How should we proceed with this patch? > >The backport to v4.9 should simply drop the last argument (NULL) in the >call to mmap_region(). > >Is there some way I can indicate this sort of thing in future patches so >that the automation can spot that I already know it won't apply cleanly >to a particular range of kernel versions? Or even better, that I could >indicate what needs to be changed when backporting to those kernel >versions? The "official" way of doing that is described here: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/stable-kernel-rules.rst#n101 However, most people just either add a comment in the commit message, or reply to the patch mail (or the "FAILED:" mail from Greg) saying how to fix this. Either way works really. Greg will also usually look up these automated mails when he adds stuff to -stable, so if you describe how to deal with it here (like you did above) is enough as well. -- Thanks, Sasha
Hello, Paul Burton wrote: > Mapping the delay slot emulation page as both writeable & executable > presents a security risk, in that if an exploit can write to & jump into > the page then it can be used as an easy way to execute arbitrary code. > > Prevent this by mapping the page read-only for userland, and using > access_process_vm() with the FOLL_FORCE flag to write to it from > mips_dsemul(). > > This will likely be less efficient due to copy_to_user_page() performing > cache maintenance on a whole page, rather than a single line as in the > previous use of flush_cache_sigtramp(). However this delay slot > emulation code ought not to be running in any performance critical paths > anyway so this isn't really a problem, and we can probably do better in > copy_to_user_page() anyway in future. > > A major advantage of this approach is that the fix is small & simple to > backport to stable kernels. > > Reported-by: Andy Lutomirski <luto@kernel.org> > Signed-off-by: Paul Burton <paul.burton@mips.com> > Fixes: 432c6bacbd0c ("MIPS: Use per-mm page to execute branch delay slot instructions") > Cc: stable@vger.kernel.org # v4.8+ Applied to mips-fixes. Thanks, Paul [ This message was auto-generated; if you believe anything is incorrect then please email paul.burton@mips.com to report it. ]
diff --git a/arch/mips/kernel/vdso.c b/arch/mips/kernel/vdso.c index 48a9c6b90e07..9df3ebdc7b0f 100644 --- a/arch/mips/kernel/vdso.c +++ b/arch/mips/kernel/vdso.c @@ -126,8 +126,8 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) /* Map delay slot emulation page */ base = mmap_region(NULL, STACK_TOP, PAGE_SIZE, - VM_READ|VM_WRITE|VM_EXEC| - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, + VM_READ | VM_EXEC | + VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC, 0, NULL); if (IS_ERR_VALUE(base)) { ret = base; diff --git a/arch/mips/math-emu/dsemul.c b/arch/mips/math-emu/dsemul.c index 5450f4d1c920..e2d46cb93ca9 100644 --- a/arch/mips/math-emu/dsemul.c +++ b/arch/mips/math-emu/dsemul.c @@ -214,8 +214,9 @@ int mips_dsemul(struct pt_regs *regs, mips_instruction ir, { int isa16 = get_isa16_mode(regs->cp0_epc); mips_instruction break_math; - struct emuframe __user *fr; - int err, fr_idx; + unsigned long fr_uaddr; + struct emuframe fr; + int fr_idx, ret; /* NOP is easy */ if (ir == 0) @@ -250,27 +251,31 @@ int mips_dsemul(struct pt_regs *regs, mips_instruction ir, fr_idx = alloc_emuframe(); if (fr_idx == BD_EMUFRAME_NONE) return SIGBUS; - fr = &dsemul_page()[fr_idx]; /* Retrieve the appropriately encoded break instruction */ break_math = BREAK_MATH(isa16); /* Write the instructions to the frame */ if (isa16) { - err = __put_user(ir >> 16, - (u16 __user *)(&fr->emul)); - err |= __put_user(ir & 0xffff, - (u16 __user *)((long)(&fr->emul) + 2)); - err |= __put_user(break_math >> 16, - (u16 __user *)(&fr->badinst)); - err |= __put_user(break_math & 0xffff, - (u16 __user *)((long)(&fr->badinst) + 2)); + union mips_instruction _emul = { + .halfword = { ir >> 16, ir } + }; + union mips_instruction _badinst = { + .halfword = { break_math >> 16, break_math } + }; + + fr.emul = _emul.word; + fr.badinst = _badinst.word; } else { - err = __put_user(ir, &fr->emul); - err |= __put_user(break_math, &fr->badinst); + fr.emul = ir; + fr.badinst = break_math; } - if (unlikely(err)) { + /* Write the frame to user memory */ + fr_uaddr = (unsigned long)&dsemul_page()[fr_idx]; + ret = access_process_vm(current, fr_uaddr, &fr, sizeof(fr), + FOLL_FORCE | FOLL_WRITE); + if (unlikely(ret != sizeof(fr))) { MIPS_FPU_EMU_INC_STATS(errors); free_emuframe(fr_idx, current->mm); return SIGBUS; @@ -282,10 +287,7 @@ int mips_dsemul(struct pt_regs *regs, mips_instruction ir, atomic_set(¤t->thread.bd_emu_frame, fr_idx); /* Change user register context to execute the frame */ - regs->cp0_epc = (unsigned long)&fr->emul | isa16; - - /* Ensure the icache observes our newly written frame */ - flush_cache_sigtramp((unsigned long)&fr->emul); + regs->cp0_epc = fr_uaddr | isa16; return 0; }
Mapping the delay slot emulation page as both writeable & executable presents a security risk, in that if an exploit can write to & jump into the page then it can be used as an easy way to execute arbitrary code. Prevent this by mapping the page read-only for userland, and using access_process_vm() with the FOLL_FORCE flag to write to it from mips_dsemul(). This will likely be less efficient due to copy_to_user_page() performing cache maintenance on a whole page, rather than a single line as in the previous use of flush_cache_sigtramp(). However this delay slot emulation code ought not to be running in any performance critical paths anyway so this isn't really a problem, and we can probably do better in copy_to_user_page() anyway in future. A major advantage of this approach is that the fix is small & simple to backport to stable kernels. Reported-by: Andy Lutomirski <luto@kernel.org> Signed-off-by: Paul Burton <paul.burton@mips.com> Fixes: 432c6bacbd0c ("MIPS: Use per-mm page to execute branch delay slot instructions") Cc: stable@vger.kernel.org # v4.8+ --- arch/mips/kernel/vdso.c | 4 ++-- arch/mips/math-emu/dsemul.c | 38 +++++++++++++++++++------------------ 2 files changed, 22 insertions(+), 20 deletions(-)