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] [day] [month] [year] [list]

Date:   Fri, 25 Aug 2017 08:13:46 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Sai Praneeth Prakhya <sai.praneeth.prakhya@...el.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        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>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>
Subject: Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually
 twiddling with cr3

On Thu, Aug 24, 2017 at 7:36 PM, Sai Praneeth Prakhya
<sai.praneeth.prakhya@...el.com> wrote:
> On Tue, 2017-08-15 at 14:46 -0700, Andy Lutomirski 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?
>>
>> Admittedly, the latter problem may well have existed even before these patches.
>
> Hi All,
>
> Could we please decouple the above issue from this patch set, so that we
> could have common efi_mm between x86 and ARM and also improve
> readability and maintainability for x86/efi.

I don't see why not.

>
> As it seems that "Everything EFI as kthread" might solve the above issue
> for real (which might take quite some time to implement, taking into
> consideration the complexity involved and some special case with
> pstore), do you think this patch set seems OK?
>
> If so, I will send out a V2 addressing the mmdropping issue.
>
> Regards,
> Sai
>