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

[Prarit Bhargava <prarit@redhat.com>]

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
>>
>