From: Andy Lutomirski <luto@kernel.org>
To: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Andy Lutomirski <luto@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
"linux-efi@vger.kernel.org" <linux-efi@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
joeyli <jlee@suse.com>, Borislav Petkov <bp@alien8.de>,
"Michael S. Tsirkin" <mst@redhat.com>,
ricardo.neri@intel.com, Matt Fleming <matt@codeblueprint.co.uk>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
"Ravi V. Shankar" <ravi.v.shankar@intel.com>
Subject: Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3
Date: Tue, 15 Aug 2017 17:47:05 -0700 [thread overview]
Message-ID: <CALCETrXTTaWb+batTfw-fKQN1gDoE=Xd2uqceSrRyRO1PoaANw@mail.gmail.com> (raw)
In-Reply-To: <1502843039.9150.19.camel@intel.com>
On Tue, Aug 15, 2017 at 5:23 PM, Sai Praneeth Prakhya
<sai.praneeth.prakhya@intel.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@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);
>>
>
> Thanks for the quick review Andy,
>
>> 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 makes sense, I will look into it.
>
>> 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.
>
> We are switching to efi_mm from user mm_struct because
> EFI_RUNTIME_SERVICES like efi_set_variable()/efi_get_variable() are
> accessible only through efi_pgd. The user thread calls ioctl() which in
> turn calls efi_call() and thus efi_switch_mm(). So, I think, the user
> still does not have direct access to EFI_RUNTIME_SERVICES memory regions
> but accesses them through sys call.
>
>> What if a perf interrupt happens while you're in the alternate mm?
>
> Since we are disabling/enabling interrupts around switching, I think we
> are safe. We do these in following functions
> phys_efi_set_virtual_address_map()
> efi_thunk_set_virtual_address_map()
> efi_call_virt_pointer()
perf uses NMI, so this doesn't help.
Perhaps the sequence could look like this:
local_irq_disable();
current->active_mm = efi_mm;
switch_to();
...
switch_to(back to old mm);
current->active_mm = old mm;
and make perf know that current->active_mm != current->mm means that
user memory is off limits.
next prev parent reply other threads:[~2017-08-16 0:47 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-15 19:18 [PATCH 0/3] Use mm_struct and switch_mm() instead of manually Sai Praneeth Prakhya
2017-08-15 19:18 ` [PATCH 1/3] efi: Use efi_mm in x86 as well as ARM Sai Praneeth Prakhya
2017-08-15 19:18 ` [PATCH 2/3] x86/efi: Replace efi_pgd with efi_mm.pgd Sai Praneeth Prakhya
2017-08-15 19:18 ` [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 Sai Praneeth Prakhya
2017-08-15 21:46 ` Andy Lutomirski
2017-08-16 0:23 ` Sai Praneeth Prakhya
2017-08-16 0:47 ` Andy Lutomirski [this message]
2017-08-16 9:31 ` Ard Biesheuvel
2017-08-16 9:53 ` Mark Rutland
2017-08-16 10:07 ` Will Deacon
2017-08-16 11:03 ` Mark Rutland
2017-08-16 12:57 ` Matt Fleming
2017-08-16 16:14 ` Andy Lutomirski
2017-08-15 22:35 ` Mark Rutland
2017-08-17 10:35 ` Will Deacon
2017-08-17 15:52 ` Andy Lutomirski
2017-08-21 10:33 ` Peter Zijlstra
2017-08-21 13:56 ` Andy Lutomirski
2017-08-21 14:08 ` Peter Zijlstra
2017-08-21 15:23 ` Andy Lutomirski
2017-08-21 15:59 ` Peter Zijlstra
2017-08-21 16:08 ` Ard Biesheuvel
2017-08-23 22:52 ` Sai Praneeth Prakhya
2017-08-25 15:13 ` Andy Lutomirski
2017-08-21 17:24 ` Peter Zijlstra
2017-08-25 2:36 ` Sai Praneeth Prakhya
2017-08-25 15:13 ` Andy Lutomirski
2017-12-17 0:06 [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with %cr3 Sai Praneeth Prakhya
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALCETrXTTaWb+batTfw-fKQN1gDoE=Xd2uqceSrRyRO1PoaANw@mail.gmail.com' \
--to=luto@kernel.org \
--cc=ard.biesheuvel@linaro.org \
--cc=bp@alien8.de \
--cc=jlee@suse.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=matt@codeblueprint.co.uk \
--cc=mst@redhat.com \
--cc=peterz@infradead.org \
--cc=ravi.v.shankar@intel.com \
--cc=ricardo.neri@intel.com \
--cc=sai.praneeth.prakhya@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).