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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c72f3ef9-ee82-84c8-c802-c9593a1dd5d1@intel.com>
Date:   Fri, 15 Sep 2023 14:30:11 +0800
From:   "Yang, Weijiang" <weijiang.yang@...el.com>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC:     "peterz@...radead.org" <peterz@...radead.org>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Gao, Chao" <chao.gao@...el.com>,
        "john.allen@....com" <john.allen@....com>
Subject: Re: [PATCH v6 03/25] x86/fpu/xstate: Add CET supervisor mode state
 support

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@intel.com/
>>
>> Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
> Otherwise, the code looked good to me.