Re: [PATCH v2] x86/split_lock: Sanitize userspace and guest error output

[Sean Christopherson <sean.j.christopherson@intel.com>]

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@intel.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
> >>
> > 
>