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
| ||
|
Date: Thu, 11 Jun 2020 13:29:54 -0400 From: Prarit Bhargava <prarit@...hat.com> To: Sean Christopherson <sean.j.christopherson@...el.com> Cc: linux-kernel@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>, Tony Luck <tony.luck@...el.com>, "Peter Zijlstra (Intel)" <peterz@...radead.org>, Rahul Tanwar <rahul.tanwar@...ux.intel.com>, Xiaoyao Li <xiaoyao.li@...el.com>, Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>, Dave Hansen <dave.hansen@...ux.intel.com> Subject: Re: [PATCH v2] x86/split_lock: Sanitize userspace and guest error output On 6/8/20 1:15 PM, Sean Christopherson wrote: > On Mon, Jun 08, 2020 at 08:21:14AM -0400, Prarit Bhargava wrote: >> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c >> index 166d7c355896..e02ec81fe1eb 100644 >> --- a/arch/x86/kernel/cpu/intel.c >> +++ b/arch/x86/kernel/cpu/intel.c >> @@ -1074,10 +1074,17 @@ static void split_lock_init(void) >> split_lock_verify_msr(sld_state != sld_off); >> } >> >> -static void split_lock_warn(unsigned long ip) >> +static bool split_lock_warn(unsigned long ip, int fatal_no_warn) >> { >> - pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n", >> - current->comm, current->pid, ip); >> + if (fatal_no_warn) >> + return false; > > This misses the point Xiaoyao was making. If EFLAGS.AC=1 then the #AC is a > legacy alignment check fault and should not be treated as a split-lock #AC. > The basic premise of the patch makes sense, but the end result is confusing > because incorporating "fatal" and the EFLAGS.AC state into split_lock_warn() > bastardizes both the "split_lock" and "warn" aspects of the function. > > E.g. something like this yields the same net effect, it's just organized > differently. If so desired, the "bogus" message could be dropped via > Xiaoyao's prep patch[*] so that this change would only affect the sld_fatal > messages. > > [*] https://lkml.kernel.org/r/20200509110542.8159-3-xiaoyao.li@intel.com > Sean, I'll just go with your patch below. It's good enough. I'll add a Signed-off-by from you as well. P. > > diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c > index 23fd5f319908..1aad0b8e394c 100644 > --- a/arch/x86/kernel/cpu/intel.c > +++ b/arch/x86/kernel/cpu/intel.c > @@ -1071,11 +1071,14 @@ static void split_lock_init(void) > split_lock_verify_msr(sld_state != sld_off); > } > > -static void split_lock_warn(unsigned long ip) > +static bool handle_split_lock(unsigned long ip) > { > pr_warn_ratelimited("#AC: %s/%d took a split_lock trap at address: 0x%lx\n", > current->comm, current->pid, ip); > > + if (sld_state != sld_warn) > + return false; > + > /* > * Disable the split lock detection for this task so it can make > * progress and set TIF_SLD so the detection is re-enabled via > @@ -1083,18 +1086,13 @@ static void split_lock_warn(unsigned long ip) > */ > sld_update_msr(false); > set_tsk_thread_flag(current, TIF_SLD); > + return true; > } > > bool handle_guest_split_lock(unsigned long ip) > { > - if (sld_state == sld_warn) { > - split_lock_warn(ip); > + if (handle_split_lock(ip)) > return true; > - } > - > - pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n", > - current->comm, current->pid, > - sld_state == sld_fatal ? "fatal" : "bogus", ip); > > current->thread.error_code = 0; > current->thread.trap_nr = X86_TRAP_AC; > @@ -1105,10 +1103,10 @@ EXPORT_SYMBOL_GPL(handle_guest_split_lock); > > bool handle_user_split_lock(struct pt_regs *regs, long error_code) > { > - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal) > + if (regs->flags & X86_EFLAGS_AC) > return false; > - split_lock_warn(regs->ip); > - return true; > + > + return handle_split_lock(regs->ip); > } > > /* > > >> + >> + pr_warn_ratelimited("#AC: %s/%d %ssplit_lock trap at address: 0x%lx\n", >> + current->comm, current->pid, >> + sld_state == sld_fatal ? "fatal " : "", ip); >> + >> + if (sld_state == sld_fatal) >> + return false; >> >> /* >> * Disable the split lock detection for this task so it can make >> @@ -1086,18 +1093,13 @@ static void split_lock_warn(unsigned long ip) >> */ >> sld_update_msr(false); >> set_tsk_thread_flag(current, TIF_SLD); >> + return true; >> } >> >> bool handle_guest_split_lock(unsigned long ip) >> { >> - if (sld_state == sld_warn) { >> - split_lock_warn(ip); >> + if (split_lock_warn(ip, 0)) >> return true; >> - } >> - >> - pr_warn_once("#AC: %s/%d %s split_lock trap at address: 0x%lx\n", >> - current->comm, current->pid, >> - sld_state == sld_fatal ? "fatal" : "bogus", ip); >> >> current->thread.error_code = 0; >> current->thread.trap_nr = X86_TRAP_AC; >> @@ -1108,10 +1110,7 @@ EXPORT_SYMBOL_GPL(handle_guest_split_lock); >> >> bool handle_user_split_lock(struct pt_regs *regs, long error_code) >> { >> - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal) >> - return false; >> - split_lock_warn(regs->ip); >> - return true; >> + return split_lock_warn(regs->ip, regs->flags & X86_EFLAGS_AC); >> } >> >> /* >> -- >> 2.21.3 >> >
Powered by blists - more mailing lists