linux-kernel - Re: [PATCH v2 0/5] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER) in .regset

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] [day] [month] [year] [list]

Message-ID: <20250829150605.GA6035@redhat.com>
Date: Fri, 29 Aug 2025 17:06:06 +0200
From: Oleg Nesterov <oleg@...hat.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "debug@...osinc.com" <debug@...osinc.com>,
	"mingo@...nel.org" <mingo@...nel.org>,
	"bp@...en8.de" <bp@...en8.de>,
	"broonie@...nel.org" <broonie@...nel.org>,
	"peterz@...radead.org" <peterz@...radead.org>,
	"hpa@...or.com" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"Mehta, Sohil" <sohil.mehta@...el.com>,
	"x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v2 0/5] x86/fpu: don't abuse x86_task_fpu(PF_USER_WORKER)
 in .regset_get() paths

On 08/28, Edgecombe, Rick P wrote:
>
> On Wed, 2025-08-27 at 16:51 +0200, Oleg Nesterov wrote:
> > >
> > > I guess I question whether it really makes sense to add a special case for
> > > PF_USER_WORKER, including the existing logic. But I'm still trying to piece
> > > together a clearly stated benefit.
> >
> > Again, I don't understand... To me, currently arch/x86/kernel/fpu/regset.c
> > adds a special case for PF_USER_WORKER, this series tries to remove it (but
> > we need a bit more of simple changes).
>
> That commit I dug up? It didn't have a super strong justification either. Can
> you say what your intended benefit is?

I meant that arch/x86/kernel/fpu/regset.c adds a special case for PF_USER_WORKER
in that this is the only case when x86_task_fpu(PF_USER_WORKER) is used.

> > OK, then how/where we can clear this flag if we avoid the pointless shadow
> > stack allocation for PF_USER_WORKER?
>
> *If* we want to worry about an extra shadow stack allocation (which Dave seems
> to doubt), we don't need to clear ARCH_SHSTK_SHSTK to avoid allocations. Other
> thread types already avoid it (vfork, etc). So just add to the existing logic
> that skips shadow stack allocation. Make it do that for user workers too, and
> leave ARCH_SHSTK_SHSTK alone.

>From 0/5:

	However, there is an annoying complication: shstk_alloc_thread_stack()
	can alloc the pointless shadow stack for PF_USER_WORKER thread and set
	the ARCH_SHSTK_SHSTK flag. This means that ssp_get()->ssp_active() can
	return true, and in this case it wouldn't be right to use the "unrelated"
	init_fpstate.

> > Why?
>
> Because ARCH_SHSTK_SHSTK is supposed to be inherited by children. It adds a
> special case for no reason.

See above. And it has no meaning for io-threads, right?

> > Plus, again, the current code is not consistent. fpu_clone() won't do
> > update_fpu_shstk() in this case. Not a bug too, but imo deserves a cleanup.
>
> I thought we discussed that the user worker logic already wipes the whole FPU
> state though, so we don't need to call update_fpu_shstk(). Did I get that wrong?

Sure, but see the note from 0/5.

We don't need to call update_fpu_shstk() and initialize ->user_ssp.
Yet ssp_get() will report the bogus cetregs->user_ssp.

This all doesn't look right to me even if nothing really bad can happen.

Oleg.