On Thu, Nov 30, 2017 at 07:38:25PM +0000, Maciej W. Rozycki wrote:
> Hi Dave,
>
> > > linux-mips-nt-prfpreg-count.diff
> > > Index: linux-sfr-test/arch/mips/kernel/ptrace.c
> > > ===================================================================
> > > --- linux-sfr-test.orig/arch/mips/kernel/ptrace.c 2017-11-21 22:12:00.000000000 +0000
> > > +++ linux-sfr-test/arch/mips/kernel/ptrace.c 2017-11-21 22:13:13.471970000 +0000
> > > @@ -484,7 +484,7 @@ static int fpr_set_msa(struct task_struc
> > > int err;
> > >
> > > BUILD_BUG_ON(sizeof(fpr_val) != sizeof(elf_fpreg_t));
> > > - for (i = 0; i < NUM_FPU_REGS && *count >= sizeof(elf_fpreg_t); i++) {
> > > + 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));
> >
> > But mips*_regsets[REGSET_FPR].size == sizeof(elf_fpreg_t),
> > linux/kernel/regset.c:ptrace_regset() polices
> > iov_len % regset->size == 0, and each user_regset_copyout() call here
> > transfers sizeof(elf_fpreg_t) bytes, decrementing *count by that
> > amount unless something goest wrong in which case we return.
>
> Good point, I missed that check.
Understandable. If took me a fair while to figure out how the logic
typically works here.
> I don't think however that re-enforcing in arch code, especially in such
> a subtle way, a constraint that has already been enforced upstream in
> generic code is a good idea, because if we ever decide to relax the
> constraint, then all the arch code will have to be carefully reviewed.
>
> > If we can't end up with that, then this patch doesn't change ABI-
> > observable behaviour, unless I've missed something.
>
> Right, in which case there is no need to backport this change if it is
> given a go-ahead.
>
> > If we can end up with that somehow, then this patch reintroduces the
> > issue d614fd58a283 aims to fix, whereby fpr_val can contain
> > uninitialised kernel stack which userspace can then obtain via
> > PTRACE_GETREGSET.
>
> That wasn't actually clarified in the referred commit's description,
> which it should in the first place, and I wasn't able to track down any
> review of your change as submitted, which would be the potential second
> source of such support information. The description isn't even correct,
> as it states that if a short buffer is supplied, then the old values held
> in thread's registers are preserved, which clearly isn't correct as
> individual registers do get written from the beginning of the regset up to
> the point no more data is available to fill a whole register.
True, the commit message leaves something to be desired here. This was
one of a bunch of patches applying similar fixes to various arches, so
I pasted the same commit message as a general description for the changes
as a set -- but you're right, it doesn't describe this spefic change
correctly here. My bad.
> 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.
> 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.
Cheers
---Dave
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
On Wed, 6 Dec 2017, Maciej W. Rozycki wrote:
> > > 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.
I take it back. After thinking about it some more in the course of
adding the commentary, I realised that with the addition of 2/5 we'll have
a logical inconsistency with the treatment of `count' in that for someone
reading this code without also looking at `ptrace_regset' it will appear
that in the case of a partial register write we call `user_regset_copyin'
to store FCSR having not completely exhausted `count' with FGR accesses.
Which means that for initial `count' being say 12 we'd put the first 8
bytes into $f0 and the following 4 bytes into FCSR rather than into $f1,
which I would find confusing if reading this code the first time. This of
course doesn't matter with our code as it is now, because it does not
access FCSR at all, however this gets changed with 2/5.
So I think that while such a change will have no effect at run time,
because we guarantee that `count % sizeof(elf_fpreg_t) == 0' elsewhere, I
think that using `count > 0' rather than `count >= sizeof(elf_fpreg_t)'
will make code more consistent. If you or anyone else feels uneasy about
`fpr_val' appearing potentially partially uninitialised (even though it
cannot trigger), then I can offer a minimal change for that, which will
preset the variable to 0.
Also to keep things consistent, this patch will have to be reordered
ahead the 2/5 FCSR fix. I will therefore submit v2 of the whole series,
with patches shuffled appropriately, BUG_ON added as previously discussed
as well as an explanatory comment.
Maciej