Re: [PATCH v2 1/8] x86/feature: Detect the x86 IBRS feature to control Speculation

[Tim Chen]

On 01/06/2018 04:56 AM, Borislav Petkov wrote:
> On Fri, Jan 05, 2018 at 06:12:16PM -0800, Tim Chen wrote:
> 
> <--- This needs an introductory sentence here.
> 
>> cpuid ax=0x7, return rdx bit 26 to indicate presence of this feature
> 
> You can write that as CPUID(7).RDX[26].
> 
>> IA32_SPEC_CTRL (0x48)
>> IA32_SPEC_CTRL, bit0 â Indirect Branch Restricted Speculation (IBRS)
> 
> Ah, those are MSRs. Please say so.
> 
>> If IBRS is set, near returns and near indirect jumps/calls will not allow
>> their predicted target address to be controlled by code that executed in
>> a less privileged prediction mode before the IBRS mode was last written
>> with a value of 1 or on another logical processor so long as all RSB
> 
> "RSB" is?

RSB is return stack buffer, basically speculation addresses for return statement.

> 
>> entries from the previous less privileged prediction mode are overwritten.
>>
>> * Thus a near indirect jump/call/return may be affected by code in a
>> less privileged prediction mode that executed AFTER IBRS mode was last
>> written with a value of 1
> 
> End sentences with a full stop.
> 
>> * There is no need to clear IBRS before writing it with a value of
>> 1. Unconditionally writing it with a value of 1 after the prediction
>> mode change is sufficient
> 
> This sounds strange. I know of funky MSRs like that but if it is not
> the case here, no need to mention it then.
> 
>> * Note: IBRS is not required in order to isolate branch predictions for
>> SMM or SGX enclaves
>>
>> * Code executed by a sibling logical processor cannot control indirect
>> jump/call/return predicted target when IBRS is set
>>
>> * SMEP will prevent supervisor mode using RSB entries filled by user code;
>> this can reduce the need for software to overwrite RSB entries
>>
>> CPU performance could be reduced when running with IBRS set.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
>> ---
>>  arch/x86/include/asm/cpufeatures.h       | 1 +
>>  arch/x86/include/asm/msr-index.h         | 4 ++++
>>  arch/x86/kernel/cpu/scattered.c          | 1 +
>>  tools/arch/x86/include/asm/cpufeatures.h | 1 +
>>  4 files changed, 7 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
>> index 07cdd17..5ee0737 100644
>> --- a/arch/x86/include/asm/cpufeatures.h
>> +++ b/arch/x86/include/asm/cpufeatures.h
>> @@ -209,6 +209,7 @@
>>  #define X86_FEATURE_AVX512_4FMAPS	( 7*32+17) /* AVX-512 Multiply Accumulation Single precision */
>>  
>>  #define X86_FEATURE_MBA			( 7*32+18) /* Memory Bandwidth Allocation */
>> +#define X86_FEATURE_SPEC_CTRL		( 7*32+19) /* Control Speculation Control */
>>  
>>  /* Virtualization flags: Linux defined, word 8 */
>>  #define X86_FEATURE_TPR_SHADOW		( 8*32+ 0) /* Intel TPR Shadow */
>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index 34c4922..f881add 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -42,6 +42,10 @@
>>  #define MSR_PPIN_CTL			0x0000004e
>>  #define MSR_PPIN			0x0000004f
>>  
>> +#define MSR_IA32_SPEC_CTRL		0x00000048
>> +#define SPEC_CTRL_FEATURE_DISABLE_IBRS	(0 << 0)
>> +#define SPEC_CTRL_FEATURE_ENABLE_IBRS	(1 << 0)
> 
> s/_FEATURE//
> 
> SPEC_CTRL_{ENABLE,DISABLE}_IBRS is good enough.
> 
>> +
>>  #define MSR_IA32_PERFCTR0		0x000000c1
>>  #define MSR_IA32_PERFCTR1		0x000000c2
>>  #define MSR_FSB_FREQ			0x000000cd
> ...
> 

Thanks. will update the phrasing.

Tim