[MODERATED] Re: [patch V11 00/16] SSB 0

[Ingo Molnar <mingo@kernel.org>]

* speck for Tim Chen <speck@linutronix.de> wrote:


> On 05/02/2018 02:51 PM, speck for Thomas Gleixner wrote:
> > Changes since V10:
> > 
> >   - Addressed Ingos review feedback
> > 
> >   - Picked up Reviewed-bys
> > 
> > Delta patch below. Bundle is coming in separate mail. Git repo branches are
> > updated as well. The master branch contains also the fix for the lost IBRS
> > issue Tim was seeing.
> > 
> > If there are no further issues and nitpicks, I'm going to make the
> > changes immutable and changes need to go incremental on top.
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> > 
> 
> I notice that this code ignores the current process's TIF_RDS setting
> in the prctl case:
> 
> #define firmware_restrict_branch_speculation_end()                      \
> do {                                                                    \
>         u64 val = x86_get_default_spec_ctrl();                          \
>                                                                         \
>         alternative_msr_write(MSR_IA32_SPEC_CTRL, val,                  \
>                               X86_FEATURE_USE_IBRS_FW);                 \
>         preempt_enable();                                               \
> } while (0)
> 
> x86_get_default_spec_ctrl will return x86_spec_ctrl_base, which
> will result in x86_spec_ctrl_base written to the MSR
> in the prctl case for Intel CPU.  That incorrectly ignores current
> process's TIF_RDS setting and the RDS bit will not be set.
> 
> Instead, the following value should have been written to the MSR
> for Intel CPU:
> x86_spec_ctrl_base | rds_tif_to_spec_ctrl(current_thread_info()->flags)

I wanted to suggest to do testing on affected and non-affected CPUs, both AMD and 
Intel, because the runtime TIF and MSR indexing logic looks sufficiently complex 
to me for it to be possibly wrong in some of the scenarios.

Thanks,

	Ingo