> On Sep 25, 2020, at 9:48 AM, Yu, Yu-cheng <yu-cheng.yu@xxxxxxxxx> wrote:
>
> On 9/25/2020 9:31 AM, Andy Lutomirski wrote:
>>> On Fri, Sep 25, 2020 at 7:58 AM Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx> wrote:
>>>
>
> [...]
>
>>> @@ -286,6 +289,37 @@ bool emulate_vsyscall(unsigned long error_code,
>>> /* Emulate a ret instruction. */
>>> regs->ip = caller;
>>> regs->sp += 8;
>>> +
>>> +#ifdef CONFIG_X86_CET
>>> + if (tsk->thread.cet.shstk_size || tsk->thread.cet.ibt_enabled) {
>>> + struct cet_user_state *cet;
>>> + struct fpu *fpu;
>>> +
>>> + fpu = &tsk->thread.fpu;
>>> + fpregs_lock();
>>> +
>>> + if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
>>> + copy_fpregs_to_fpstate(fpu);
>>> + set_thread_flag(TIF_NEED_FPU_LOAD);
>>> + }
>>> +
>>> + cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER);
>>> + if (!cet) {
>>> + fpregs_unlock();
>>> + goto sigsegv;
>> I *think* your patchset tries to keep cet.shstk_size and
>> cet.ibt_enabled in sync with the MSR, in which case it should be
>> impossible to get here, but a comment and a warning would be much
>> better than a random sigsegv.
>
> Yes, it should be impossible to get here. I will add a comment and a warning, but still do sigsegv. Should this happen, and the function return, the app gets a control-protection fault. Why not let it fail early?
I’m okay with either approach as long as we get a comment and warning.
>
>>
>> Shouldn't we have a get_xsave_addr_or_allocate() that will never
>> return NULL but instead will mark the state as in use and set up the
>> init state if the feature was previously not in use?
>
> We already have a static __raw_xsave_addr(), which returns a pointer to the requested xstate. Maybe we can export __raw_xsave_addr(), if that is needed.
I don’t think that’s what we want in general — we want the whole construct of initializing the state if needed.
