linux-kernel - Re: [PATCH 4/5] MIPS: Execute any partial write of the last register with PTRACE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.DEB.2.00.1712061859120.4584@tp.orcam.me.uk>
Date:   Wed, 6 Dec 2017 19:24:36 +0000
From:   "Maciej W. Rozycki" <macro@...s.com>
To:     Dave Martin <Dave.Martin@....com>
CC:     Ralf Baechle <ralf@...ux-mips.org>,
        James Hogan <james.hogan@...s.com>,
        Paul Burton <Paul.Burton@...s.com>,
        Alex Smith <alex@...x-smith.me.uk>,
        "linux-mips@...ux-mips.org" <linux-mips@...ux-mips.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>
Subject: Re: [PATCH 4/5] MIPS: Execute any partial write of the last register
 with PTRACE_SETREGSET

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