From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752685AbdHOVq5 (ORCPT ); Tue, 15 Aug 2017 17:46:57 -0400 Received: from mail.kernel.org ([198.145.29.99]:39664 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752627AbdHOVqz (ORCPT ); Tue, 15 Aug 2017 17:46:55 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org C49DA22C99 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=luto@kernel.org MIME-Version: 1.0 In-Reply-To: <1502824706-30762-4-git-send-email-sai.praneeth.prakhya@intel.com> References: <1502824706-30762-1-git-send-email-sai.praneeth.prakhya@intel.com> <1502824706-30762-4-git-send-email-sai.praneeth.prakhya@intel.com> From: Andy Lutomirski Date: Tue, 15 Aug 2017 14:46:33 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 To: Sai Praneeth Prakhya , Peter Zijlstra Cc: "linux-efi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , joeyli , Borislav Petkov , Andrew Lutomirski , "Michael S. Tsirkin" , ricardo.neri@intel.com, Matt Fleming , Ard Biesheuvel , "Ravi V. Shankar" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 15, 2017 at 12:18 PM, Sai Praneeth Prakhya 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. > +} > + > #ifdef CONFIG_EFI_MIXED > extern efi_status_t efi64_thunk(u32, ...); > > @@ -649,16 +665,13 @@ efi_status_t efi_thunk_set_virtual_address_map( > efi_sync_low_kernel_mappings(); > local_irq_save(flags); > > - efi_scratch.prev_cr3 = read_cr3(); > - write_cr3((unsigned long)efi_scratch.efi_pgt); > - __flush_tlb_all(); > + efi_switch_mm(&efi_mm); > > func = (u32)(unsigned long)phys_set_virtual_address_map; > status = efi64_thunk(func, memory_map_size, descriptor_size, > descriptor_version, virtual_map); > > - write_cr3(efi_scratch.prev_cr3); > - __flush_tlb_all(); > + efi_switch_mm(efi_scratch.prev_mm); > local_irq_restore(flags); > > return status;