On Fri, 1 Dec 2017, Dave Martin wrote:
> > You are of course right about the (partially) uninitialised variable, and
> > I think there are two ways to address it:
> >
> > 1. By preinitialising it, i.e.:
> >
> > for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > fpr_val = get_fpr64(&target->thread.fpu.fpr[i], 0);
> > err = user_regset_copyin(pos, count, kbuf, ubuf,
> > &fpr_val, i * sizeof(elf_fpreg_t),
> > (i + 1) * sizeof(elf_fpreg_t));
> > if (err)
> > return err;
> > set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> > }
> >
> > but that would be an overkill given that we assert that `count' is a
> > multiple of `sizeof(elf_fpreg_t)'.
>
> Agreed.
>
> Was a partial write to fscr ever supported by this regset? Your commit
> message suggested that my patch may have broken that, but I can't see
> how it was ever possible in the first place... unless .size has been
> changed for this regset at some point.
>
> If my patch does cause an ABI regression, then it certainly should be
> fixed though.
I have looked into it some more, and I can see the upstream check:
if (!regset || (kiov->iov_len % regset->size) != 0)
return -EIO;
has always been there since the introduction of the regset feature, which
was with commit 2225a122ae26 ("ptrace: Add support for generic
PTRACE_GETREGSET/PTRACE_SETREGSET"). And the core file writer, i.e.
`fill_thread_core_info', always operates on whole regsets by taking the
size from the regset definition in question itself.
Which means that my patch does not change the situation, but as far as
security is concerned neither did yours, as you addressed an impossible
case. You did improve performance a bit though for the corner case
situation of a partial regset access, by avoiding iterating over further
FPRs with a call to `user_regset_copyin' once the requested transfer size
has been exhausted.
> > 2. Actually assert what we rely on having been enforced by generic code,
> > i.e.:
> >
> > BUG_ON(*count % sizeof(elf_fpreg_t));
> > for (i = 0; i < NUM_FPU_REGS && *count > 0; i++) {
> > err = user_regset_copyin(pos, count, kbuf, ubuf,
> > &fpr_val, i * sizeof(elf_fpreg_t),
> > (i + 1) * sizeof(elf_fpreg_t));
> > if (err)
> > return err;
> > set_fpr64(&target->thread.fpu.fpr[i], 0, fpr_val);
> > }
> >
> > so that a discrepancy between generic code and the arch handler is
> > caught should it happen.
>
> The important property is that *count is at least sufficient to fill
> fpr_val, so that a zero return user_regset_copyin() means fpr_val has
> been fully initialised with user data.
>
> So while your check is not wrong
>
> (since *count > 0 && *count % sizeof(elf_fpreg_t) == 0 implies
> *count >= sizeof(elf_fpreg_t))
>
> I don't see how this is an improvement on the original check.
>
>
> Either way, maybe adding a comment to explain the purpose of the check
> would be a good idea.
I agree a comment is worth having here given the complex dependency.
Therefore, taking what has been observed in the course of this discussion
into account and unless either you or someone else has further input I am
going to withdraw this patch from the series as recorded in patchwork and
submit another one separately that only adds a comment quoting the
observations made. Obviously it won't be a backport candidate, but 5/5
does not depend on this change, so I believe there is no need to
regenerate and repost the remaining changes from this series.
Thank you for your input.
Maciej