lkml.org 
[lkml]   [2017]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3
    On Wed, Aug 16, 2017 at 11:07:10AM +0100, Will Deacon wrote:
    > On Wed, Aug 16, 2017 at 10:53:38AM +0100, Mark Rutland wrote:
    > > On Wed, Aug 16, 2017 at 10:31:12AM +0100, Ard Biesheuvel wrote:
    > > > (+ Mark, Will)
    > > >
    > > > On 15 August 2017 at 22:46, Andy Lutomirski <luto@kernel.org> wrote:
    > > > > On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
    > > > > <sai.praneeth.prakhya@intel.com> wrote:
    > > > >> +/*
    > > > >> + * Makes the calling kernel thread switch to/from efi_mm context
    > > > >> + * Can be used from SetVirtualAddressMap() or during efi runtime calls
    > > > >> + * (Note: This routine is heavily inspired from use_mm)
    > > > >> + */
    > > > >> +void efi_switch_mm(struct mm_struct *mm)
    > > > >> +{
    > > > >> + struct task_struct *tsk = current;
    > > > >> +
    > > > >> + task_lock(tsk);
    > > > >> + efi_scratch.prev_mm = tsk->active_mm;
    > > > >> + if (efi_scratch.prev_mm != mm) {
    > > > >> + mmgrab(mm);
    > > > >> + tsk->active_mm = mm;
    > > > >> + }
    > > > >> + switch_mm(efi_scratch.prev_mm, mm, NULL);
    > > > >> + task_unlock(tsk);
    > > > >> +
    > > > >> + if (efi_scratch.prev_mm != mm)
    > > > >> + mmdrop(efi_scratch.prev_mm);
    > > > >
    > > > > I'm confused. You're mmdropping an mm that you are still keeping a
    > > > > pointer to. This is also a bit confusing in the case where you do
    > > > > efi_switch_mm(efi_scratch.prev_mm).
    > > > >
    > > > > This whole manipulation seems fairly dangerous to me for another
    > > > > reason -- you're taking a user thread (I think) and swapping out its
    > > > > mm to something that the user in question should *not* have access to.
    > > > > What if a perf interrupt happens while you're in the alternate mm?
    > > > > What if you segfault and dump core? Should we maybe just have a flag
    > > > > that says "this cpu is using a funny mm", assert that the flag is
    > > > > clear when scheduling, and teach perf, coredumps, etc not to touch
    > > > > user memory when the flag is set?
    > > >
    > > > It appears we may have introduced this exact issue on arm64 and ARM by
    > > > starting to run the UEFI runtime services with interrupts enabled.
    > > > (perf does not use NMI on ARM, so the issue did not exist beforehand)
    > > >
    > > > Mark, Will, any thoughts?
    > >
    > > Yup, I can cause perf to take samples from the EFI FW code, so that's
    > > less than ideal.
    >
    > But that should only happen if you're profiling EL1, right, which needs
    > root privileges? (assuming the skid issue is solved -- not sure what
    > happened to those patches after they broke criu).

    I *think* that only needs perf_event_paranoid < 1, rather than root.

    It's certianly not accessible by default to most users (e.g. my Ubuntu
    fs sets this to 2, and IIRC Debian go to a much more stringent
    non-upstream paranoid level).

    > > The "funny mm" flag sounds like a good idea to me, though given recent
    > > pain with sampling in the case of skid, I don't know exactly what we
    > > should do if/when we take an overflow interrupt while in EFI.
    >
    > I don't think special-casing perf interrupts is the right thing to do here.
    > If we're concerned about user-accesses being made off the back of interrupts
    > taken whilst in EFI, then we should probably either swizzle back in the
    > user page table on the IRQ path or postpone handling it until we're done
    > with the firmware.

    Doing that for every IRQ feels odd, especially as the result would be
    sampling something that wasn't executed, potentially repeatedly, giveing
    bogus info.

    > Having a flag feels a bit weird: would the uaccess routines return
    > -EFAULT if it's set?

    I'd expect we'd abort at a higher level, not taking any sample. i.e.
    we'd have the core overflow handler check in_funny_mm(), and if so, skip
    the sample, as with the skid case.

    Thanks,
    Mark.

    \
     
     \ /
      Last update: 2017-08-16 13:05    [W:3.138 / U:0.516 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site