Re: [PATCH] x86/lib: Use REP MOVSB for large memcpy() on ERMS CPUs without FSRM

[Usama Arif]

[...]

>>>> diff --git a/arch/x86/lib/memcpy_64.S b/arch/x86/lib/memcpy_64.S
>>>> index 12a23fa7c44c5..407b226d4de2f 100644
>>>> --- a/arch/x86/lib/memcpy_64.S
>>>> +++ b/arch/x86/lib/memcpy_64.S
>>>> @@ -31,24 +31,27 @@
>>>>   * which the compiler could/should do much better anyway.
>>>>   */
>>>>  SYM_TYPED_FUNC_START(__memcpy)
>>>> -	ALTERNATIVE "jmp memcpy_orig", "", X86_FEATURE_FSRM
>>>> -
>>>>  	movq %rdi, %rax
>>>>  	movq %rdx, %rcx
>>>> -	rep movsb
>>>> -	RET
>>>> -SYM_FUNC_END(__memcpy)
>>>> -EXPORT_SYMBOL(__memcpy)
>>>> -
>>>> -SYM_FUNC_ALIAS_MEMFUNC(memcpy, __memcpy)
>>>> -SYM_PIC_ALIAS(memcpy)
>>>> -EXPORT_SYMBOL(memcpy)
>>>>  
>>>> -SYM_FUNC_START_LOCAL(memcpy_orig)
>>>> -	movq %rdi, %rax
>>>> +#ifdef CONFIG_MITIGATION_ITS
>>>> +	/*
>>>> +	 * Returns are patched before alternatives. Keep the replacement
>>>> +	 * return in the first half of a cache line so ITS retains its thunk
>>>> +	 * when the instruction is copied to its final address.  
>>>
>>> I'm not 100% sure what that is about, but does it mean that the cache line
>>> alignment of the code here also matters?  
>>
>> Not the alignment of __memcpy() itself, only that of the replacement in
>> .altinstr_replacement.
>>
>> On ITS affected CPUs, a RET in the lower 32 bytes of a 64-byte cache
>> line needs mitigation, so apply_returns() leaves such sites as a jump to
>> its_return_thunk, whose own RET is in the upper half, and turns the rest
>> into a plain RET. It runs before apply_alternatives(), so for the RET in
>> the FSRM replacement it decides from the replacement's address, not from
>> where the code is copied to. A plain RET could then end up in the lower
>> half of a line in __memcpy().
> 
> So the RET in the replacement is in the first half a cache line- so becomes a ret.
> But, unless __memcpy() is aligned, the relocated RET might be in the second
> half so would need mitigating - but you copy in a mitigation-free ret.

It's the other way round: a RET in the first half keeps the jump to
its_return_thunk, see cpu_wants_rethunk_at():

        return !((unsigned long)addr & 0x20);

So the copy is the thunk jump, which is safe in either half.

> 
> If that is the only mitigation then wouldn't aligning memcpy mean you could
> just use a ret (possibly followed by an 'int 3' - or whatever is the 'flavour
> of the month' instruction for stopping SLS).

It isn't the only one: with the retbleed, SRSO or call depth tracking
thunk every return needs it, so the code has to use RET and let
apply_returns() pick. For ITS alone, aligning __memcpy() would save one
direct jump on FSRM CPUs with ITS (Ice Lake to Rocket Lake), which I
don't think is worth it.

[...]