[PATCH v2 2/4] arm64/signal: Include TPIDR2 in the signal context
Catalin Marinas
catalin.marinas at arm.com
Fri Nov 18 11:10:16 PST 2022
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
More information about the linux-arm-kernel
mailing list