linux-kernel - Re: linux-next: build warning after merge of the suspend tree

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: <pm-qos-needs-fixing@mdm.bga.com>
Date:	Mon, 23 May 2011 11:40:23 -0500
From:	Milton Miller <miltonm@....com>
To:	mgross <markgross@...gnar.org>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>, linux-next@...r.kernel.org,
	linux-kernel@...r.kernel.org, Randy Dunlap <rdunlap@...otime.net>,
	Stephen Rothwell <sfr@...b.auug.org.au>
Subject: Re: linux-next: build warning after merge of the suspend tree

[ I managed to botch my own email the first time, how embarrassing!
So I added Randy and updated the paragraph about negative values. ]

On Mon, 23 May 2011 about 14:18:46 -0000, mgross wrote:
> On Mon, May 23, 2011 at 03:06:36PM +1000, Stephen Rothwell wrote:
> > Hi Rafael,
> > 
> > After merging the suspend tree, today's linux-next build (i386 defconfig
> > among others) produced this warning:
> > 
> > kernel/pm_qos_params.c: In function 'pm_qos_power_write':
> > kernel/pm_qos_params.c:420: warning: passing argument 3 of 'kstrtol' from incompatible pointer type
> > include/linux/kernel.h:210: note: expected 'long int *' but argument is of type 's32 *'
> > 
> > Intreoduced by commit 365daa955e03 ("PM: Correct PM QOS's user mode
> > interface to work with ascii input per").
> 
> Gah!  I'm sorry about that.
> 
> attached is a fix.
> 
> 
> --mark
> 
> signed-off-by:markgross <markgross@...gnar.org>
> 

(1) This should be in the patch, not the enclosing letter
(2) Incorrect capitalization
(3) Incorrect spacing

Please read Documentation/SubmittingPatches again.

> 
> 
> >From a8f0587b9ae598be5ca4c3cdda4e0ced6ca9baaf Mon Sep 17 00:00:00 2001
> From: mgross <mgross@...8>
> Date: Mon, 23 May 2011 07:14:09 -0700
> Subject: [PATCH] clean up a compile time warning in the use of strict_strtol but that was
>  passing an s32 * when it should be passing a long *
> 

>From should match Signed-off-by:

Please seperate title (subject) and description body

Maybe: pm_qos: strict_strtol takes a long, not s32

strict_strtol takes a pointer to long to store the converted value.
introduced in xxxx ("change set title here")

So that the reviewers can quickly see if it needs to be backported
to stable etc.

except read below

> ---
> kernel/pm_qos_params.c |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index d61ecf3..dd37c56 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -405,6 +405,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  		size_t count, loff_t *f_pos)
>  {
>  	s32 value;
> +	long safe_int;
>  	int x;
>  	char ascii_value[11];
>  	struct pm_qos_request_list *pm_qos_req;
> @@ -417,10 +418,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  		ascii_value[count] = 0;
>  		if (copy_from_user(ascii_value, buf, count))
>  			return -EFAULT;
> -		if ((x=strict_strtol(ascii_value, 16, &value)) != 0){
> -			pr_debug("%s, 0x%x, 0x%x\n",ascii_value, value, x);
> +		if ((x=strict_strtol(ascii_value, 16, &safe_int)) != 0){

Why are you doing an assignment in the if?  Why not assign first and
compare later?

> +			pr_debug("%s, 0x%lx, 0x%x\n",ascii_value, safe_int, x);
>  			return -EINVAL;

Nit: Some reason not to return -ERANGE if thats what strtol returned?  
While folding the error to -EINVAL is ok, it hides diagnostic informatio
from the user.

>  		}
> +		value = (s32) safe_int;

You call strict checking, which includes overflow checking, but
only that the value fits in a long.  You then defeat that checking
by casting to int.

It looks like you want strict_strtoint except thats not defined.
Hoever, the pattern for strict_strto* is kstrto* and kstrtos32 is
defined ...

>  	} else
>  		return -EINVAL;
>  

Oh, and you now may copy 11 characters from userspace into an 11
character buffer then terminate it by writing the 12th character
(ascii_value[count == 11]).  Except its an 11 character array.

The variable is a s32, aparently in native endian if pased in binary
as 4 bytes.  What is the magic to set the value to a negative number
through the ascii interface?  Is yet another character for the -
required?

I see the string from userspace wasn't properly terminated before
either.  In ed77134bfccf5e75b (PM QOS update), merged in v2.6.35-rc1,
11 bytes were copied from user then passed to ssscanf without null
termination forced.  It was updated in 0109c2c48d (PM QoS: Correct
pr_debug() misuse and improve parameter checks), which was merged in
2.6.36-rc4, to change the the function that walks off the string from
sscanf to strlen.  That changelog isn't marked for stable (I didn't
look if it was sent) but it still isn't force terminated.

happy patching,
milton
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/