linux-kernel - Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKv+Gu8AySQSXtRWfHA4y5DbH-DQ7jpqkp=tS+snSTar_sKBJw@mail.gmail.com>
Date:   Wed, 16 Aug 2017 10:31:12 +0100
From:   Ard Biesheuvel <ard.biesheuvel@...aro.org>
To:     Andy Lutomirski <luto@...nel.org>,
        Will Deacon <will.deacon@....com>,
        Mark Rutland <mark.rutland@....com>
Cc:     Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>,
        Peter Zijlstra <peterz@...radead.org>,
        "linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        joeyli <jlee@...e.com>, Borislav Petkov <bp@...en8.de>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        "Neri, Ricardo" <ricardo.neri@...el.com>,
        Matt Fleming <matt@...eblueprint.co.uk>,
        "Ravi V. Shankar" <ravi.v.shankar@...el.com>
Subject: Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually
 twiddling with cr3

(+ Mark, Will)

On 15 August 2017 at 22:46, Andy Lutomirski <luto@...nel.org> wrote:
> On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya
> <sai.praneeth.prakhya@...el.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?