On 9/15/2023 8:06 AM, Edgecombe, Rick P wrote:
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
Add supervisor mode state support within FPU xstate management
framework.
Although supervisor shadow stack is not enabled/used today in
kernel,KVM
^ Nit: needs a space
requires the support because when KVM advertises shadow stack feature
to
guest, architechturally it claims the support for both user and
^ Spelling: "architecturally"
Thank you!!
supervisor
modes for Linux and non-Linux guest OSes.
With the xstate support, guest supervisor mode shadow stack state can
be
properly saved/restored when 1) guest/host FPU context is swapped
2) vCPU
thread is sched out/in.
(2) is a little bit confusing, because the lazy FPU stuff won't always
save/restore while scheduling.
It's true for normal thread, but for vCPU thread, it's a bit different, on the path to
vm-entry, after host/guest fpu states swapped, preemption is not disabled and
vCPU thread could be sched out/in, in this case, guest FPU states will be saved/
restored because TIF_NEED_FPU_LOAD is always cleared after swap.
But trying to explain the details in
this commit log is probably unnecessary. Maybe something like?
2) At the proper times while other tasks are scheduled
I just want to justify that enabling of supervisor xstate is necessary for guest.
Maybe I need to reword a bit :-)
I think also a key part of this is that XFEATURE_CET_KERNEL is not
*all* of the "guest supervisor mode shadow stack state", at least with
respect to the MSRs. It might be worth calling that out a little more
loudly.
OK, I will call it out that supervisor mode shadow stack state also includes IA32_S_CET msr.
The alternative is to enable it in KVM domain, but KVM maintainers
NAKed
the solution. The external discussion can be found at [*], it ended
up
with adding the support in kernel instead of KVM domain.
Note, in KVM case, guest CET supervisor state i.e.,
IA32_PL{0,1,2}_MSRs,
are preserved after VM-Exit until host/guest fpstates are swapped,
but
since host supervisor shadow stack is disabled, the preserved MSRs
won't
hurt host.
It might beg the question of if this solution will need to be redone by
some future Linux supervisor shadow stack effort. I *think* the answer
is no.
AFAICT KVM needs to be modified if host shadow stack is implemented, at least
guest/host CET supervisor MSRs should be swapped at the earliest time after
vm-exit so that host won't misbehavior on *guest* MSR contents.
Most of the xsave managed features are restored before returning to
userspace because they would have userspace effect. But
XFEATURE_CET_KERNEL is different. It only effects the kernel. But the
IA32_PL{0,1,2}_MSRs are used when transitioning to those rings. So for
Linux they would get used when transitioning back from userspace. In
order for it to be used when control transfers back *from* userspace,
it needs to be restored before returning *to* userspace. So despite
being needed only for the kernel, and having no effect on userspace, it
might need to be swapped/restored at the same time as the rest of the
FPU state that only affects userspace.
You're right, for enabling of supervisor mode shadow stack, we need to take
it carefully whenever ring/stack is switching. But we still have time to figure out
the points.
Thanks a lot for bring up such kind of thinking!
Probably supervisor shadow stack for Linux needs much more analysis,
but trying to leave some breadcrumbs on the thinking from internal
reviews. I don't know if it might be good to include some of this
reasoning in the commit log. It's a bit hand wavy.
IMO, we have put much assumption on the fact that CET supervisor shadow stack is not
enabled in kernel and this patch itself is straightforward and simple, it's just a small
brick for enabling supervisor shadow stack, we would revisit whether something is an
issue based on how SSS is implemented in kernel. So let's not add such kind of reasoning :-)
Thank you for the enlightenment!
[*]:
https://lore.kernel.org/all/806e26c2-8d21-9cc9-a0b7-7787dd231729@xxxxxxxxx/
Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
Otherwise, the code looked good to me.
