linux-kernel - Re: [PATCH v9 14/26] x86/arch_prctl: Create ARCH_SET_STATE_ENABLE/ARCH_GET_STATE

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: <E9C8AF5E-E229-4BA2-AEC8-4289EF7428CA@intel.com>
Date:   Mon, 9 Aug 2021 22:08:19 +0000
From:   "Bae, Chang Seok" <chang.seok.bae@...el.com>
To:     "Macieira, Thiago" <thiago.macieira@...el.com>
CC:     "bp@...e.de" <bp@...e.de>, "Lutomirski, Andy" <luto@...nel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...nel.org" <mingo@...nel.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "Brown, Len" <len.brown@...el.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 14/26] x86/arch_prctl: Create
 ARCH_SET_STATE_ENABLE/ARCH_GET_STATE_ENABLE

On Aug 6, 2021, at 09:46, Macieira, Thiago <thiago.macieira@...el.com> wrote:
> On Friday, 30 July 2021 07:59:45 PDT Chang S. Bae wrote:
>> +       for_each_thread(tsk, t) {
>> +               t->thread.fpu.dynamic_state_perm |= req_dynstate_perm;
>> +               nr_threads++;
>> +       }
>> +
>> +       if (nr_threads != tsk->signal->nr_threads) {
>> +               for_each_thread(tsk, t)
>> +                       t->thread.fpu.dynamic_state_perm =
>> old_dynstate_perm; 
>> +               pr_err("x86/fpu: ARCH_XSTATE_PERM failed
>> as thread number mismatched.\n"); 
>> +               return -EBUSY;
>> +       }
>> +       return 0;
>> +}

<snip>

> First the simpler one: that EBUSY. It must go and you can do that with a lock. 
> Library code cannot ensure that it is running in single-threaded state and 
> that no other threads are started or exit while they make the system call. 
> There's nothing the library in question can do if it got an EBUSY. Do you want 
> me to try again? What if it fails again? What's the state of the dynamically 
> permitted states after an EBUSY? It's probably inconsistent. Moreover, there's 
> an ABA problem there: what happens if a thread starts and another exits while 
> this system call is running? And what happens if two threads are making this 
> system call? 
> (also, shouldn't tsk->signal->nr_threads be an atomic read?)

I suspect the EBUSY situation is somewhat imaginative. In theory, the
situation might be possible one thread calls this syscall at some point when a
new task is being created -- after task data duplication [1] and before
enlisted [2].

As stated in the changelog, the alternative is possible:
> An alternative implementation would not save the permission bitmap in
> every task. But instead would extend the per-process signal data, and
> that would not be subject to this race.
But it involves quite a bit of code complexity and this is pretty much
backend. I think it is possible to follow up and update when the case ever
turns out to be real. At least, I'm not aware of any report against the
PR_SET_FP_MODE prctl(2) [3] which took the same way -- walk and update the
task list.

Perhaps, the hunk above can be improved to be atomic.

<snip>

> So I have to insist that the XGETBV instruction's result match exactly what is 
> permitted to run. That means we either enable AMX unconditionally with no need 
> for system calls (with or without XFD trapping to dynamically allocate more 
> state), or that the XCR0 register be set without the AMX bits by default, 
> until the system call is issued.

XCR0 provokes VMEXIT which will impact the performance hardly. At least the
opt-in model is a consensus out of the long debate [4]. Let alone the question
on how well advertise this new syscall though.

Thanks,
Chang

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/fork.c#n2128
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/fork.c#n2320
[3]: https://man7.org/linux/man-pages/man2/prctl.2.html
[4]: https://lore.kernel.org/lkml/CALCETrW2QHa2TLvnUuVxAAheqcbSZ-5_WRXtDSAGcbG8N+gtdQ@mail.gmail.com/