Re: [PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context

[Catalin Marinas <catalin.marinas@arm.com>]

On Fri, Nov 18, 2022 at 01:55:53PM +0000, Will Deacon wrote:
> On Tue, Nov 15, 2022 at 04:42:14PM +0000, Mark Brown wrote:
> > On Tue, Nov 15, 2022 at 12:22:26PM +0000, Will Deacon wrote:
> > > On Tue, Nov 15, 2022 at 12:08:44PM +0000, Mark Brown wrote:
> > > > On Mon, Nov 14, 2022 at 04:10:06PM +0000, Will Deacon wrote:
> > > > > On Mon, Oct 31, 2022 at 08:17:34PM +0000, Mark Brown wrote:
> > 
> > > > > > +			if (user->tpidr2)
> > > > > > +				goto invalid;
> > 
> > > > > > +			if (size != sizeof(*user->tpidr2))
> > 
> > > > > Why are you requiring an exact match here? Won't that hinder any future
> > > > > extension of the structure?
> > 
> > > > It will but since the structure is explicitly for a single sysreg
> > > > that's intentional - the thinking was to just continue to model
> > > > any more sysregs we want to report in the signal context in the
> > > > same format with their own contexts.  It felt like it fit better
> > > > into how everything else in the signal context is extended.
> > 
> > > I see, but having the usual '<' check wouldn't preclude us from doing
> > > what you suggest above, whilst also giving us some flexibility in case
> > > things turn out differently from how we expected.
> > 
> > This actually also how we validate the base fpsimd_context -
> > while there is a < check in the switch statement in
> > parse_user_sigframe() but we also have an exact size check
> > near the top of restore_fpsimd_context() which gets called from
> > there, meaning that the check in parse_user_sigframe() is a bit
> > redundant.  We do however allow the varibly sized frames to have
> > an oversized allocation, though those have internal sizing
> > information whereas fpsimd_context doesn't.  My take was that we
> > were erroring out here because if userspace thinks it's supplying
> > some state that we're ignoring and not restoring then things
> > might go badly.  I'm not super wedded to this approach but it is
> > consistent with the fpsimd_context handling and I can see some
> > justificaton for it being done the way it is.
> 
> Hmm, good point about fpsimd, it looks at magic/size twice which is
> definitely wrong (userspace could even change those values in between!).
> 
> So I'd vote for removing the checks from restore_fpsimd_context() which
> raises the same question we were discussing initially: should the check
> in parse_user_sigframe() require an exact size match or instead truncate
> the structure on the stack by only copying a prefix into the kernel?
> 
> I'm actually warming more towards an exact check now that we've spoken
> about it a bit... What do you think?

I'd go for an exact match as well. I don't think we can expand these
structures in the future safely without an additional magic number.

I tend to agree with Mark here that parse_user_sigframe() should only
check the magic numbers and set the corresponding user_ctxs members. We
leave the exact size check to the restore_fpsimd_context() etc. (can
skip the magic check here). Well, not a strong view either way but we
should definitely remove the duplicate checks.

-- 
Catalin

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