| 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 10:46:36 -0700
From: Sean Christopherson <sean.j.christopherson@...el.com>
To: Prarit Bhargava <prarit@...hat.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 Thu, Jun 11, 2020 at 01:37:19PM -0400, Prarit Bhargava wrote:
>
>
> 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 will just take your patch to make things easy. I will add you as a
> Signed-off-by.
>
> /me is testing the patch right now
Sure, here's an official SOB if it happens to work.
Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>
> > 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