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"
> 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. 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 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.
>
> 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.
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.
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.
>
> [*]:
> https://lore.kernel.org/all/[email protected]/
>
> Signed-off-by: Yang Weijiang <[email protected]>
Otherwise, the code looked good to me.
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/[email protected]/
>>
>> Signed-off-by: Yang Weijiang <[email protected]>
> Otherwise, the code looked good to me.
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 <[email protected]>
Best regards,
Maxim Levitsky
>
> Thank you for the enlightenment!
> > > [*]:
> > > https://lore.kernel.org/all/[email protected]/
> > >
> > > Signed-off-by: Yang Weijiang <[email protected]>
> > Otherwise, the code looked good to me.