Re: [PATCH v6 03/25] x86/fpu/xstate: Add CET supervisor mode state support

[Maxim Levitsky <mlevitsk@redhat.com>]

On Fri, 2023-09-15 at 14:30 +0800, Yang, Weijiang wrote:
> 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.

I agree.

> 
> > 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 :-)

Overall the patch looks OK to me.

Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky

> 
> Thank you for the enlightenment!
> > > [*]:
> > > https://lore.kernel.org/all/806e26c2-8d21-9cc9-a0b7-7787dd231729@intel.com/
> > > 
> > > Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> > Otherwise, the code looked good to me.