linux-kernel - Re: [PATCH v13 06/30] powerpc/ptrace: Adapt gpr32_get, gpr32

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]

Date:	Thu, 04 Aug 2016 18:28:09 +1000
From:	Michael Ellerman <mpe@...erman.id.au>
To:	Daniel Axtens <dja@...ens.net>, wei.guo.simon@...il.com,
	linuxppc-dev@...ts.ozlabs.org, linux-kernel@...r.kernel.org
Cc:	Shuah Khan <shuahkh@....samsung.com>,
	Anton Blanchard <anton@...ba.org>,
	Cyril Bur <cyrilbur@...il.com>,
	Anshuman Khandual <khandual@...ux.vnet.ibm.com>,
	Simon Guo <wei.guo.simon@...il.com>,
	Ulrich Weigand <ulrich.weigand@...ibm.com>,
	Michael Neuling <mikey@...ling.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kees Cook <keescook@...omium.org>,
	Rashmica Gupta <rashmicy@...il.com>,
	Khem Raj <raj.khem@...il.com>, Jessica Yu <jeyu@...hat.com>,
	Jiri Kosina <jkosina@...e.cz>, Miroslav Benes <mbenes@...e.cz>,
	Suraj Jitindar Singh <sjitindarsingh@...il.com>,
	Chris Smart <chris@...troguy.com>,
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v13 06/30] powerpc/ptrace: Adapt gpr32_get, gpr32_set functions for transaction

Daniel Axtens <dja@...ens.net> writes:

> [ Unknown signature status ]
> Hi all,
>
> This is causing cppcheck warnings (having just landed in next):
>
> [arch/powerpc/kernel/ptrace.c:2062]: (error) Uninitialized variable: ckpt_regs
> [arch/powerpc/kernel/ptrace.c:2130]: (error) Uninitialized variable: ckpt_regs

Sigh.

> This is from...
>> -static int gpr32_get(struct task_struct *target,
>> +static int gpr32_get_common(struct task_struct *target,
>>  		     const struct user_regset *regset,
>>  		     unsigned int pos, unsigned int count,
>> -		     void *kbuf, void __user *ubuf)
>> +			    void *kbuf, void __user *ubuf, bool tm_active)
>>  {
>>  	const unsigned long *regs = &target->thread.regs->gpr[0];
>> +	const unsigned long *ckpt_regs;
>>  	compat_ulong_t *k = kbuf;
>>  	compat_ulong_t __user *u = ubuf;
>>  	compat_ulong_t reg;
>>  	int i;
>>  
>> -	if (target->thread.regs == NULL)
>> -		return -EIO;
>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
>> +	ckpt_regs = &target->thread.ckpt_regs.gpr[0];
>> +#endif
>> +	if (tm_active) {
>> +		regs = ckpt_regs;
> ... this bit here. If the ifdef doesn't trigger, cppcheck can't find an
> initialisation for ckpt_regs, so it complains.
>
> Techinically it's a false positive as (I assume!) tm_active cannot ever
> be true in the absense of CONFIG_PPC_TRANSACTIONAL_MEM.

That's correct, so the code is safe. See the one call site (which passes an int!):

  #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
  static int tm_cgpr32_get(struct task_struct *target,
  		     const struct user_regset *regset,
  		     unsigned int pos, unsigned int count,
  		     void *kbuf, void __user *ubuf)
  {
  	return gpr32_get_common(target, regset, pos, count, kbuf, ubuf, 1);
  }

> Is there a nice simple fix we could deploy to squash this warning, or
> will we just live with it?

This series has been nothing but pain. Given we're already at v13, and people
really want this support to go in, I'm going to leave it in the tree.

Once it's in we can refactor the implementation, which is a bit of a mess, and
hopefully in the process fix the warnings.

cheers