From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753546AbdHUN4I (ORCPT ); Mon, 21 Aug 2017 09:56:08 -0400 Received: from mail-qt0-f182.google.com ([209.85.216.182]:32786 "EHLO mail-qt0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753008AbdHUN4G (ORCPT ); Mon, 21 Aug 2017 09:56:06 -0400 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (1.0) Subject: Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 From: Andy Lutomirski X-Mailer: iPhone Mail (14G60) In-Reply-To: <20170821103359.jt2xf2cx5wxjldau@hirez.programming.kicks-ass.net> Date: Mon, 21 Aug 2017 06:56:01 -0700 Cc: Andy Lutomirski , Will Deacon , Mark Rutland , Matt Fleming , Ard Biesheuvel , Sai Praneeth Prakhya , "linux-efi@vger.kernel.org" , "linux-kernel@vger.kernel.org" , joeyli , Borislav Petkov , "Michael S. Tsirkin" , "Neri, Ricardo" , "Ravi V. Shankar" Message-Id: References: <20170816095338.GB17270@leverpostej> <20170816100709.GG12845@arm.com> <20170816110321.GC17270@leverpostej> <20170816125715.GB3384@codeblueprint.co.uk> <20170815223541.GA25778@remoulade> <20170817103514.GC27872@arm.com> <20170821103359.jt2xf2cx5wxjldau@hirez.programming.kicks-ass.net> To: Peter Zijlstra Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by nfs id v7LDuEUt001822 > On Aug 21, 2017, at 3:33 AM, Peter Zijlstra wrote: > >> On Thu, Aug 17, 2017 at 08:52:38AM -0700, Andy Lutomirski wrote: >> On Thu, Aug 17, 2017 at 3:35 AM, Will Deacon wrote: > >>> I'm still concerned that we're treating perf specially here -- are we >>> absolutely sure that nobody else is going to attempt user accesses off the >>> back of an interrupt? >> >> Reasonably sure? If nothing else, an interrupt taken while mmap_sem() >> is held for write that tries to access user memory is asking for >> serious trouble. There are still a few callers of pagefault_disable() >> and copy...inatomic(), though. > > I'm not immediately seeing how holding mmap_sem for writing is a > problem. > >>> If not, then I'd much prefer a solution that catches >>> anybody doing that with the EFI page table installed, rather than trying >>> to play whack-a-mole like this. >> >> Using a kernel thread solves the problem for real. Anything that >> blindly accesses user memory in kernel thread context is terminally >> broken no matter what. > > So perf-callchain doesn't do it 'blindly', it wants either: > > - user_mode(regs) true, or > - task_pt_regs() set. > > However I'm thinking that if the kernel thread has ->mm == &efi_mm, the > EFI code running could very well have user_mode(regs) being true. > > intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are > accessible. It bails on error though. So while its careful, it does > attempt to access the 'user' mapping directly. Which should also trigger > with the EFI code. > > And I'm not seeing anything particularly broken with either. The PEBS > fixup relies on the CPU having just executed the code, and if it could > fetch and execute the code, why shouldn't it be able to fetch and read? There are two ways this could be a problem. One is that u privileged user apps shouldn't be able to read from EFI memory. The other is that, if EFI were to have IO memory mapped at a "user" address, perf could end up reading it. > (eXecute implies Read assumed). And like said, it if triggers a fault, > it bails, no worries. > > It really doesn't care if the task is a kernel thread or not. Same for > the unwinder, if we get an interrupt register set that points into > 'userspace' we try and unwind it. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andy Lutomirski Subject: Re: [PATCH 3/3] x86/efi: Use efi_switch_mm() rather than manually twiddling with cr3 Date: Mon, 21 Aug 2017 06:56:01 -0700 Message-ID: References: <20170816095338.GB17270@leverpostej> <20170816100709.GG12845@arm.com> <20170816110321.GC17270@leverpostej> <20170816125715.GB3384@codeblueprint.co.uk> <20170815223541.GA25778@remoulade> <20170817103514.GC27872@arm.com> <20170821103359.jt2xf2cx5wxjldau@hirez.programming.kicks-ass.net> Mime-Version: 1.0 (1.0) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <20170821103359.jt2xf2cx5wxjldau-Nxj+rRp3nVydTX5a5knrm8zTDFooKrT+cvkQGrU6aU0@public.gmane.org> Sender: linux-efi-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Peter Zijlstra Cc: Andy Lutomirski , Will Deacon , Mark Rutland , Matt Fleming , Ard Biesheuvel , Sai Praneeth Prakhya , "linux-efi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , joeyli , Borislav Petkov , "Michael S. Tsirkin" , "Neri, Ricardo" , "Ravi V. Shankar" List-Id: linux-efi@vger.kernel.org > On Aug 21, 2017, at 3:33 AM, Peter Zijlstra wrote: >=20 >> On Thu, Aug 17, 2017 at 08:52:38AM -0700, Andy Lutomirski wrote: >> On Thu, Aug 17, 2017 at 3:35 AM, Will Deacon wrote:= >=20 >>> I'm still concerned that we're treating perf specially here -- are we >>> absolutely sure that nobody else is going to attempt user accesses off t= he >>> back of an interrupt? >>=20 >> Reasonably sure? If nothing else, an interrupt taken while mmap_sem() >> is held for write that tries to access user memory is asking for >> serious trouble. There are still a few callers of pagefault_disable() >> and copy...inatomic(), though. >=20 > I'm not immediately seeing how holding mmap_sem for writing is a > problem. >=20 >>> If not, then I'd much prefer a solution that catches >>> anybody doing that with the EFI page table installed, rather than trying= >>> to play whack-a-mole like this. >>=20 >> Using a kernel thread solves the problem for real. Anything that >> blindly accesses user memory in kernel thread context is terminally >> broken no matter what. >=20 > So perf-callchain doesn't do it 'blindly', it wants either: >=20 > - user_mode(regs) true, or > - task_pt_regs() set. >=20 > However I'm thinking that if the kernel thread has ->mm =3D=3D &efi_mm, th= e > EFI code running could very well have user_mode(regs) being true. >=20 > intel_pmu_pebs_fixup() OTOH 'blindly' assumes that the LBR addresses are > accessible. It bails on error though. So while its careful, it does > attempt to access the 'user' mapping directly. Which should also trigger > with the EFI code. >=20 > And I'm not seeing anything particularly broken with either. The PEBS > fixup relies on the CPU having just executed the code, and if it could > fetch and execute the code, why shouldn't it be able to fetch and read? There are two ways this could be a problem. One is that u privileged user a= pps shouldn't be able to read from EFI memory. The other is that, if EFI we= re to have IO memory mapped at a "user" address, perf could end up reading i= t. > (eXecute implies Read assumed). And like said, it if triggers a fault, > it bails, no worries. >=20 > It really doesn't care if the task is a kernel thread or not. Same for > the unwinder, if we get an interrupt register set that points into > 'userspace' we try and unwind it.