Messages in this thread | | | Date | Fri, 19 Nov 2021 01:27:13 +0800 | Subject | Re: [PATCH V5 01/50] x86/entry: Add fence for kernel entry swapgs in paranoid_entry() | From | Lai Jiangshan <> |
| |
On 2021/11/18 23:54, Peter Zijlstra wrote:
> > I'm confused, shouldn't the LFENCE be between SWAPGS and future uses of > GS prefix?
I'm wrong a again.
I once thought "it should be followed with serializing operations such as SWITCH_TO_KERNEL_CR3", and tglx corrected me:
https://lore.kernel.org/lkml/875yumbgox.ffs@tglx/
> It does not matter whether the *serializing* is before or after
And in my brain, it was incorrectly stored as: It does not matter whether the *fence* is before or after.
I will update patch1 and the corresponding C code in later patches.
Patch 1 in V4 is correct, but not as good as Borislav Petkov pointed out that it has duplicated FENCE_SWAPGS_KERNEL_ENTRY.
I will change it as
rdmsr if (need_swapgs) { swapgs set ebx/return value } FENCE_SWAPGS_KERNEL_ENTRY
> > In the old code, before 96b2371413e8f, we had: > > swapgs > SAVE_AND_SWITCH_TO_KERNEL_CR3 > FENCE_SWAPGS_KERNEL_ENTRY > > // %gs user comes here.. > > And the comment made sense, since if SAVE_AND_SWITCH_TO_KERNEL_CR3 would > imply an unconditional CR3 write, the LFENCE would not be needed. > > Then along gomes 96b2371413e8f and changes the order to: > > SAVE_AND_SWITCH_TO_KERNEL_CR3 > swapgs > FENCE_SWAPGS_KERNEL_ENTRY > // %gs user comes here.. > > But now the comment is crazy talk, because even if the CR3 write were > unconditional, it'd be pointless, since it's not after SWAPGS, but we > still have the LFENCE in the right place.
I think the comments also make sense.
If CR3 write were unconditional before swapgs, no fence is needed after swapgs since cr3 write is serializing.
> > But now you want to make it: > > SAVE_AND_SWITCH_TO_KERNEL_CR3 > FENCE_SWAPGS_KERNEL_ENTRY > swapgs > // %gs user comes here.. > > And there's nothing left and speculation can use the old %gs for our > user and things go sideways. Hmm? > > > (on a completely unrelated note, I find KERNEL_ENTRY and USER_ENTRY > utterly confusing) >
| |