From: Lai Jiangshan <[email protected]>
Commit 18ec54fdd6d18 ("x86/speculation: Prepare entry code for Spectre
v1 swapgs mitigations") (commit1) added FENCE_SWAPGS_{KERNEL|USER}_ENTRY
for conditional swapgs. And in paranoid_entry(), it uses only
FENCE_SWAPGS_KERNEL_ENTRY for both conditions/branches. It is totally
correct because FENCE_SWAPGS_KERNEL_ENTRY implies FENCE_SWAPGS_USER_ENTRY
which can be seen in spectre_v1_select_mitigation() that if
X86_FEATURE_FENCE_SWAPGS_USER is set, X86_FEATURE_FENCE_SWAPGS_KERNEL
must be also set.
The commit1 also has a piece of comment saying why
FENCE_SWAPGS_KERNEL_ENTRY is needed since writing cr3 implies lfence:
writing cr3 is also conditionally.
But commit 96b2371413e8f ("x86/entry/64: Switch CR3 before SWAPGS in
paranoid entry") (commit2) switches the code order and at least this piece
of comments is useless because there is no "writing cr3" in between the
conditional swaps and the fence.
Even worse, the commit2 does not use FENCE_SWAPGS_{KERNEL|USER}_ENTRY
in the corresponding branches. It just uses FENCE_SWAPGS_KERNEL_ENTRY
in the user path and no FENCE_SWAPGS_KERNEL_ENTRY in the kernel path.
Possibly, it was because the commit1 which uses FENCE_SWAPGS_KERNEL_ENTRY
in both paths and shadowed the lfence requirement.
Fix it and remove the unneeded comment.
Fixes: Commit 96b2371413e8f ("x86/entry/64: Switch CR3 before SWAPGS in paranoid entry")
Cc: Josh Poimboeuf <[email protected]>
Cc: Chang S. Bae <[email protected]>
Cc: Sasha Levin <[email protected]>
Signed-off-by: Lai Jiangshan <[email protected]>
---
arch/x86/entry/entry_64.S | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index e38a4cf795d9..95d85b16710b 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -898,17 +898,12 @@ SYM_CODE_START_LOCAL(paranoid_entry)
rdmsr
testl %edx, %edx
jns .Lparanoid_entry_swapgs
+ FENCE_SWAPGS_KERNEL_ENTRY
ret
.Lparanoid_entry_swapgs:
swapgs
-
- /*
- * The above SAVE_AND_SWITCH_TO_KERNEL_CR3 macro doesn't do an
- * unconditional CR3 write, even in the PTI case. So do an lfence
- * to prevent GS speculation, regardless of whether PTI is enabled.
- */
- FENCE_SWAPGS_KERNEL_ENTRY
+ FENCE_SWAPGS_USER_ENTRY
/* EBX = 0 -> SWAPGS required on exit */
xorl %ebx, %ebx
--
2.19.1.6.gb485710b
Lai,
On Sun, Sep 26 2021 at 23:07, Lai Jiangshan wrote:
> --- a/arch/x86/entry/entry_64.S
> +++ b/arch/x86/entry/entry_64.S
> @@ -898,17 +898,12 @@ SYM_CODE_START_LOCAL(paranoid_entry)
> rdmsr
> testl %edx, %edx
> jns .Lparanoid_entry_swapgs
> + FENCE_SWAPGS_KERNEL_ENTRY
Good catch.
> ret
>
> .Lparanoid_entry_swapgs:
> swapgs
> -
> - /*
> - * The above SAVE_AND_SWITCH_TO_KERNEL_CR3 macro doesn't do an
> - * unconditional CR3 write, even in the PTI case. So do an lfence
> - * to prevent GS speculation, regardless of whether PTI is enabled.
> - */
> - FENCE_SWAPGS_KERNEL_ENTRY
> + FENCE_SWAPGS_USER_ENTRY
This change is wrong.
In the paranoid entry path even if user GS base is set then the entry
does not necessarily come from user space so there is no guarantee that
there was a CR3 write on PTI enabled systems before the SWAPGS.
FENCE_SWAPGS_USER_ENTRY does not emit a LFENCE when PTI is enabled, so
both the comment and FENCE_SWAPGS_KERNEL_ENTRY which emits LFENCE on
affected CPUs unconditionaly are correct. Though the comment could do
with some polishing to make this entirely clear.
Before adding support for FSGSBASE both the swapgs and non swapgs case
issued the LFENCE unconditionally with FENCE_SWAPGS_KERNEL_ENTRY. The
commit you identified splitted the code pathes and failed to add the
FENCE_SWAPGS_KERNEL_ENTRY into the non-swapgs path.
Thanks,
tglx
On 2021/9/27 04:43, Thomas Gleixner wrote:
> Lai,
>
> On Sun, Sep 26 2021 at 23:07, Lai Jiangshan wrote:
>> --- a/arch/x86/entry/entry_64.S
>> +++ b/arch/x86/entry/entry_64.S
>> @@ -898,17 +898,12 @@ SYM_CODE_START_LOCAL(paranoid_entry)
>> rdmsr
>> testl %edx, %edx
>> jns .Lparanoid_entry_swapgs
>> + FENCE_SWAPGS_KERNEL_ENTRY
>
> Good catch.
>
>> ret
>>
>> .Lparanoid_entry_swapgs:
>> swapgs
>> -
>> - /*
>> - * The above SAVE_AND_SWITCH_TO_KERNEL_CR3 macro doesn't do an
>> - * unconditional CR3 write, even in the PTI case. So do an lfence
>> - * to prevent GS speculation, regardless of whether PTI is enabled.
>> - */
>> - FENCE_SWAPGS_KERNEL_ENTRY
>> + FENCE_SWAPGS_USER_ENTRY
>
> This change is wrong.
>
> In the paranoid entry path even if user GS base is set then the entry
> does not necessarily come from user space so there is no guarantee that
> there was a CR3 write on PTI enabled systems before the SWAPGS.
>
> FENCE_SWAPGS_USER_ENTRY does not emit a LFENCE when PTI is enabled, so
> both the comment and FENCE_SWAPGS_KERNEL_ENTRY which emits LFENCE on
> affected CPUs unconditionaly are correct. Though the comment could do
> with some polishing to make this entirely clear.
I didn't notice FENCE_SWAPGS_USER_ENTRY depends on PTI.
I will add FENCE_SWAPGS_KERNEL_ENTRY only on the kernel path.
Thanks
Lai
On 2021/9/27 09:10, Lai Jiangshan wrote:
>>
>> This change is wrong.
>>
>> In the paranoid entry path even if user GS base is set then the entry
>> does not necessarily come from user space so there is no guarantee that
>> there was a CR3 write on PTI enabled systems before the SWAPGS.
>>
>> FENCE_SWAPGS_USER_ENTRY does not emit a LFENCE when PTI is enabled, so
>> both the comment and FENCE_SWAPGS_KERNEL_ENTRY which emits LFENCE on
>> affected CPUs unconditionaly are correct. Though the comment could do
>> with some polishing to make this entirely clear.
>
>
> I didn't notice FENCE_SWAPGS_USER_ENTRY depends on PTI.
The commit c75890700455 ("x86/entry/64: Remove unneeded kernel CR3 switching")
( https://lore.kernel.org/all/[email protected]/ )
also made it wrong.
When the SWITCH_TO_KERNEL_CR3 in the path is removed, FENCE_SWAPGS_USER_ENTRY
should also be changed to FENCE_SWAPGS_KERNEL_ENTRY. (Or just jmp to
.Lerror_entry_done_lfence which has FENCE_SWAPGS_KERNEL_ENTRY already.)
And FENCE_SWAPGS_USER_ENTRY could be documented with "it should be followed with
serializing operations such as SWITCH_TO_KERNEL_CR3". Or we can add a
SWAPGS_AND_SWITCH_TO_KERNEL_CR3 to combine them.
I will fix it in v3. (Or should I do it separately before v3?)
Sorry for my fault.
Lai
>
> I will add FENCE_SWAPGS_KERNEL_ENTRY only on the kernel path.
>
> Thanks
> Lai
Lai,
On Mon, Sep 27 2021 at 11:27, Lai Jiangshan wrote:
> On 2021/9/27 09:10, Lai Jiangshan wrote:
>
> The commit c75890700455 ("x86/entry/64: Remove unneeded kernel CR3 switching")
> ( https://lore.kernel.org/all/[email protected]/ )
> also made it wrong.
Duh, did not spot that either.
> When the SWITCH_TO_KERNEL_CR3 in the path is removed, FENCE_SWAPGS_USER_ENTRY
> should also be changed to FENCE_SWAPGS_KERNEL_ENTRY. (Or just jmp to
> .Lerror_entry_done_lfence which has FENCE_SWAPGS_KERNEL_ENTRY already.)
Yes.
> And FENCE_SWAPGS_USER_ENTRY could be documented with "it should be followed with
> serializing operations such as SWITCH_TO_KERNEL_CR3".
It does not matter whether the serializing is before or after. The
problem is:
if (from_user)
swapgs();
can take the wrong path speculatively which means the speculation is
then based on the wrong GS.
We have these sequences in the non paranoid entries:
if (from_user) {
pti_switch_cr3();
swapgs();
}
if (from_user) {
swapgs();
pti_switch_cr3();
}
and with mitigation these become:
if (from_user) {
pti_switch_cr3();
swapgs();
lfence_if_not_pti();
} else {
lfence();
}
if (from_user) {
swapgs();
lfence_if_not_pti();
pti_switch_cr3();
} else {
lfence();
}
When PTI is enabled then the CR3 write is sufficient because it's fully
serializing. If PTI is off the LFENCE is required. On which side the CR3
write is before or after SWAPGS does not matter.
> Or we can add a SWAPGS_AND_SWITCH_TO_KERNEL_CR3 to combine them.
No. We really don't want to go there.
Thanks,
tglx