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.1711301831540.31156@tp.orcam.me.uk>
Date:   Thu, 30 Nov 2017 19:38:25 +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

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.

 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.

 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)'.

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.

 Thoughts?

  Maciej