Re: [PATCH] arm64/signal: Don't assume that TIF_SVE means we saved SVE state

From: Dave Martin <Dave.Martin@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Will Deacon <will@kernel.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, stable@vger.kernel.org
Subject: Re: [PATCH] arm64/signal: Don't assume that TIF_SVE means we saved SVE state
Date: Tue, 30 Jan 2024 15:47:26 +0000	[thread overview]
Message-ID: <ZbkaDjyuYVEct+kl@e133380.arm.com> (raw)
In-Reply-To: <7027bd80-1fea-493a-83d7-ffef4e548f08@sirena.org.uk>

On Tue, Jan 30, 2024 at 02:53:45PM +0000, Mark Brown wrote:
> On Tue, Jan 30, 2024 at 02:44:51PM +0000, Dave Martin wrote:
> > On Tue, Jan 30, 2024 at 02:09:34PM +0000, Mark Brown wrote:
> 
> > > That said if this is preempted ptrace *could* come in and rewrite the
> > > data or at worst change the vector length (which could leave us with
> > > sve_state deallocated or a different size, possibly while we're in the
> > > middle of accessing it).  This could also happen with the existing check
> > > for TIF_SVE so I don't think there's anything new here - AFAICT this has
> > > always been an issue with the vector code, unless I'm missing some
> > > bigger thing which excludes ptrace.  I think any change that's needed
> > > there won't overlap with this one, I'm looking.
> 
> > I'm pretty sure that terrible things will happen treewide if ptrace can
> > ever access or manipulate the internal state of a _running_ task.
> 
> > I think the logic is that any ptrace call that can access or manipulate
> > the state of a task is gated on the task being ptrace-stopped.  Once we
> 
> ...
> 
> > I haven't tracked down the smokeproof gun in the code yet, though.
> 
> Yes, exactly - this feels like something that surely must be handled
> already with exclusion along the lines that you're describing but I
> didn't yet spot exactly what the mechanism is.

I think the critical thing is the task_is_traced() check in
kernel/ptrace.c.  This seems to be what gates every ptrace call that
requires a traced task (i.e., stopped under ptrace).

So long as nothing during the delivery of a single signal can trigger a
ptrace-stop itself, I think ptrace can't get in the middle of it.  This
would imply calling the signal delivery code recursively (not just
raising one signal while delivering another).  I'd be pretty confident
that this is meant to be impossible from a design standpoint.

> > From memory, I think that the above forced flush was there to protect
> > against the context switch code rather than ptrace, and guarantees that
> > any change that ctxsw _might_ spontaneously make to the task state has
> > already been done and dusted before we do the actual signal delivery.
> > This may be a red herring so far as ptrace hazards are concerned.
> 
> Indeed, it's all about the current task and won't help at for ptrace.

Agreed

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel