[PATCH] PM QoS: Allow parsing of ASCII values

[PATCH] PM QoS: Allow parsing of ASCII values

[Simon Horman]

In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
the parsing of the ASCII hex value was tightened. Unfortunately
it was tightened to the point where no value is valid.

Root of the problem seems to lie in wheather the ASCII hex is followed
by a '\n' or not. My reading of the documentation is that the '\n' should
not be present. However the code previously only accepted that version.
The current code accepts neither. My fix is to accept both.

Cc: Mark Gross <markgross@thegnar.org>
Cc: Dan Carpenter <error27@gmail.com>
Cc: Rafael J. Wysocki <rjw@sisk.pl>
Signed-off-by: Simon Horman <horms@verge.net.au>

---
This appears to have been introduced around 2.6.36-rc4.
And was an @stable patch. As such I believe this change
is stable material.
---
 kernel/pm_qos_params.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index aeaa7f8..98a34ea 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -387,10 +387,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 	if (count == sizeof(s32)) {
 		if (copy_from_user(&value, buf, sizeof(s32)))
 			return -EFAULT;
-	} else if (count == 11) { /* len('0x12345678/0') */
-		if (copy_from_user(ascii_value, buf, 11))
+	} else if (count == 11 || count == 10) { /* len('0x12345678\n') or
+						  * len('0x12345678') */
+		if (copy_from_user(ascii_value, buf, count))
 			return -EFAULT;
-		if (strlen(ascii_value) != 10)
+		if (strlen(ascii_value) != count)
 			return -EINVAL;
 		x = sscanf(ascii_value, "%x", &value);
 		if (x != 1)
-- 
1.7.2.3

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[mark gross]

On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> the parsing of the ASCII hex value was tightened. Unfortunately
> it was tightened to the point where no value is valid.

How is it of no value?  Can you not sent a 10 char string with a
zero-byte end of string maker?

Is expecting the c-string marker in the interface a bad idea that needs
fixing?
 
> Root of the problem seems to lie in wheather the ASCII hex is followed
> by a '\n' or not. My reading of the documentation is that the '\n' should
> not be present. However the code previously only accepted that version.
> The current code accepts neither. My fix is to accept both.

The documentation says it should be a 10byte string.
"Alternatively the user mode program could write a hex string for the
value using 10 char long format e.g. "0x12345678".  This translates to a
pm_qos_update_request call."

When I wrote the code I was thinking that a c-string that has a zero
byte end of string maker.  note: a new line character is not implied
anywhere.

However; reading the documentation it is somewhat ambiguous the buffer
is 10 bytes or 11.

We should tighten up the Documentation and make the code match it.

I don't like the 10 or 11 byte buffer logic.  It should be one or the
other and not include any new line character.

What do you want the documentation to read?

--mark

> 
> Cc: Mark Gross <markgross@thegnar.org>
> Cc: Dan Carpenter <error27@gmail.com>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> This appears to have been introduced around 2.6.36-rc4.
> And was an @stable patch. As such I believe this change
> is stable material.
> ---
>  kernel/pm_qos_params.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index aeaa7f8..98a34ea 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -387,10 +387,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  	if (count == sizeof(s32)) {
>  		if (copy_from_user(&value, buf, sizeof(s32)))
>  			return -EFAULT;
> -	} else if (count == 11) { /* len('0x12345678/0') */
> -		if (copy_from_user(ascii_value, buf, 11))
> +	} else if (count == 11 || count == 10) { /* len('0x12345678\n') or
> +						  * len('0x12345678') */
> +		if (copy_from_user(ascii_value, buf, count))
>  			return -EFAULT;
> -		if (strlen(ascii_value) != 10)
> +		if (strlen(ascii_value) != count)
>  			return -EINVAL;
>  		x = sscanf(ascii_value, "%x", &value);
>  		if (x != 1)
> -- 
> 1.7.2.3
> 

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[Simon Horman]

Hi Mark,

On Thu, Feb 17, 2011 at 09:05:16PM -0800, mark gross wrote:
> On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> > In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> > the parsing of the ASCII hex value was tightened. Unfortunately
> > it was tightened to the point where no value is valid.
> 
> How is it of no value?  Can you not sent a 10 char string with a
> zero-byte end of string maker?

My testing indicates that

echo -n "0x12345678" will yield count == 10 in copy_from_user()

that

echo "0x12345678" will yield count == 11 in copy_from_user()

and that in both case strlen() will yeild the same value as count.

> Is expecting the c-string marker in the interface a bad idea that needs
> fixing?

I think that is the crux of the problem from my point of view.
That is, using echo as above there is no '\0' by the time
the string gets to the kernel.

I now see that the following works :-)

echo -ne '0x12345678\0' | dd of=/dev/network_latency

However I wonder if the call to strlen() is safe.
In the case of the following is there any guarantee that
ascii_value is '\0' terminated? Perhaps strnlen() is needed?

echo -n '0x123456789' | dd of=/dev/network_latency

> > Root of the problem seems to lie in wheather the ASCII hex is followed
> > by a '\n' or not. My reading of the documentation is that the '\n' should
> > not be present. However the code previously only accepted that version.
> > The current code accepts neither. My fix is to accept both.
> 
> The documentation says it should be a 10byte string.
> "Alternatively the user mode program could write a hex string for the
> value using 10 char long format e.g. "0x12345678".  This translates to a
> pm_qos_update_request call."
> 
> When I wrote the code I was thinking that a c-string that has a zero
> byte end of string maker.  note: a new line character is not implied
> anywhere.
> 
> However; reading the documentation it is somewhat ambiguous the buffer
> is 10 bytes or 11.
> 
> We should tighten up the Documentation and make the code match it.
> 
> I don't like the 10 or 11 byte buffer logic.  It should be one or the
> other and not include any new line character.
> 
> What do you want the documentation to read?

I would describe the format as

"Alternatively the user mode program could write a hex string for the
value using 11 char long format e.g. "0x12345678\0". Note that the trailing
'\0' needs to be included in the write to the device."

Because to me the existing description implies

	str = "0x12345678";
	write(fd, str, srlen(str));

But the correct code is

	str = "0x12345678";
	write(fd, str, srlen(str) + 1);

To be honest I think that the first code-snippet is more in keeping with
typical C string usage. But to change the code to only accept that would
break compatibility.

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[Alan Stern]

On Fri, 18 Feb 2011, Simon Horman wrote:

> Hi Mark,
> 
> On Thu, Feb 17, 2011 at 09:05:16PM -0800, mark gross wrote:
> > On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> > > In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> > > the parsing of the ASCII hex value was tightened. Unfortunately
> > > it was tightened to the point where no value is valid.
> > 
> > How is it of no value?  Can you not sent a 10 char string with a
> > zero-byte end of string maker?
> 
> My testing indicates that
> 
> echo -n "0x12345678" will yield count == 10 in copy_from_user()
> 
> that
> 
> echo "0x12345678" will yield count == 11 in copy_from_user()
> 
> and that in both case strlen() will yeild the same value as count.
> 
> > Is expecting the c-string marker in the interface a bad idea that needs
> > fixing?
> 
> I think that is the crux of the problem from my point of view.
> That is, using echo as above there is no '\0' by the time
> the string gets to the kernel.
> 
> I now see that the following works :-)
> 
> echo -ne '0x12345678\0' | dd of=/dev/network_latency
> 
> However I wonder if the call to strlen() is safe.
> In the case of the following is there any guarantee that
> ascii_value is '\0' terminated? Perhaps strnlen() is needed?
> 
> echo -n '0x123456789' | dd of=/dev/network_latency

Normally these interfaces are designed so that people can do either

	echo '...data...' >/dev/...

or

	echo -n '...data...' >/dev/...

and it will just work.  This means you should allow count to be equal 
to either 10 or 11, but if it is 11 then you should check that the last 
character really is '\n'.

Also, you might want to consider using strict_strtoul() for parsing or 
conversion.

Alan Stern

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[mark gross]

On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> the parsing of the ASCII hex value was tightened. Unfortunately
> it was tightened to the point where no value is valid.
> 
> Root of the problem seems to lie in wheather the ASCII hex is followed
> by a '\n' or not. My reading of the documentation is that the '\n' should
> not be present. However the code previously only accepted that version.
> The current code accepts neither. My fix is to accept both.
> 
> Cc: Mark Gross <markgross@thegnar.org>
> Cc: Dan Carpenter <error27@gmail.com>
> Cc: Rafael J. Wysocki <rjw@sisk.pl>
> Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> ---
> This appears to have been introduced around 2.6.36-rc4.
> And was an @stable patch. As such I believe this change
> is stable material.
> ---
>  kernel/pm_qos_params.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index aeaa7f8..98a34ea 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -387,10 +387,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  	if (count == sizeof(s32)) {
>  		if (copy_from_user(&value, buf, sizeof(s32)))
>  			return -EFAULT;
> -	} else if (count == 11) { /* len('0x12345678/0') */
> -		if (copy_from_user(ascii_value, buf, 11))
> +	} else if (count == 11 || count == 10) { /* len('0x12345678\n') or
shouldn't this be count ==12 ||count ==10?
After taking Alan's advice and looking at strict_strtoul it looks like
10 and 12 are the numbers to use.

Also playing with your dd test:
mgross@mgt:~$ echo -n 0x12345678 | dd of=junk.bin
0+1 records in
0+1 records out
10 bytes (10 B) copied, 0.000368621 s, 27.1 kB/s
mgross@mgt:~$ hexdump junk.bin 
0000000 7830 3231 3433 3635 3837               
000000a
mgross@mgt:~$ echo  0x12345678 | dd of=junk.bin
0+1 records in
0+1 records out
11 bytes (11 B) copied, 0.000384755 s, 28.6 kB/s
mgross@mgt:~$ hexdump junk.bin 
0000000 7830 3231 3433 3635 3837 000a          
000000b

it looks like I have 10 or 12 bytes (but can't reconcile the dd output
saying 11 bytes when hexdump is showing 12 )

The following patch (untested) should make things work ok.


diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index aeaa7f8..6cbce91 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -381,19 +381,18 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 {
 	s32 value;
 	int x;
-	char ascii_value[11];
+	char ascii_value[12];
 	struct pm_qos_request_list *pm_qos_req;
 
 	if (count == sizeof(s32)) {
 		if (copy_from_user(&value, buf, sizeof(s32)))
 			return -EFAULT;
-	} else if (count == 11) { /* len('0x12345678/0') */
-		if (copy_from_user(ascii_value, buf, 11))
+	} else if (count == 10 || count == 12) { /* '0x12345678' or
+						    '0x12345678/n/0'*/
+		memset(ascii_value, 0, sizeof(ascii_value));
+		if (copy_from_user(ascii_value, buf, count))
 			return -EFAULT;
-		if (strlen(ascii_value) != 10)
-			return -EINVAL;
-		x = sscanf(ascii_value, "%x", &value);
-		if (x != 1)
+		if (strict_strtoul(ascii_value,16,value) != 0)
 			return -EINVAL;
 		pr_debug("%s, %d, 0x%x\n", ascii_value, x, value);
 	} else

------
I'll test this tomorrow and do a proper posting (if it works right)

--mark
> +						  * len('0x12345678') */
> +		if (copy_from_user(ascii_value, buf, count))
>  			return -EFAULT;
> -		if (strlen(ascii_value) != 10)
> +		if (strlen(ascii_value) != count)
>  			return -EINVAL;
>  		x = sscanf(ascii_value, "%x", &value);
>  		if (x != 1)
> -- 
> 1.7.2.3
> 

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[mark gross]

On Mon, Feb 21, 2011 at 08:33:36PM -0800, mark gross wrote:
> On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> > In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> > the parsing of the ASCII hex value was tightened. Unfortunately
> > it was tightened to the point where no value is valid.
> > 
> > Root of the problem seems to lie in wheather the ASCII hex is followed
> > by a '\n' or not. My reading of the documentation is that the '\n' should
> > not be present. However the code previously only accepted that version.
> > The current code accepts neither. My fix is to accept both.
> > 
> > Cc: Mark Gross <markgross@thegnar.org>
> > Cc: Dan Carpenter <error27@gmail.com>
> > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > ---
> > This appears to have been introduced around 2.6.36-rc4.
> > And was an @stable patch. As such I believe this change
> > is stable material.
> > ---
> >  kernel/pm_qos_params.c |    7 ++++---
> >  1 files changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index aeaa7f8..98a34ea 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -387,10 +387,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> >  	if (count == sizeof(s32)) {
> >  		if (copy_from_user(&value, buf, sizeof(s32)))
> >  			return -EFAULT;
> > -	} else if (count == 11) { /* len('0x12345678/0') */
> > -		if (copy_from_user(ascii_value, buf, 11))
> > +	} else if (count == 11 || count == 10) { /* len('0x12345678\n') or
> shouldn't this be count ==12 ||count ==10?
> After taking Alan's advice and looking at strict_strtoul it looks like
> 10 and 12 are the numbers to use.
> 
> Also playing with your dd test:
> mgross@mgt:~$ echo -n 0x12345678 | dd of=junk.bin
> 0+1 records in
> 0+1 records out
> 10 bytes (10 B) copied, 0.000368621 s, 27.1 kB/s
> mgross@mgt:~$ hexdump junk.bin 
> 0000000 7830 3231 3433 3635 3837               
> 000000a
> mgross@mgt:~$ echo  0x12345678 | dd of=junk.bin
> 0+1 records in
> 0+1 records out
> 11 bytes (11 B) copied, 0.000384755 s, 28.6 kB/s
> mgross@mgt:~$ hexdump junk.bin 
> 0000000 7830 3231 3433 3635 3837 000a          
> 000000b

> 
> it looks like I have 10 or 12 bytes (but can't reconcile the dd output
> saying 11 bytes when hexdump is showing 12 )

on a 64 bit ubuntu box its 11 bytes that get sent to the kernel hexdump
must be padding the extra byte.> 
> The following patch (untested) should make things work ok.
it doesn't work and I'm still debugging some straingess with  strict_strtoul

it looks like its munging the string I'm passing in.

> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index aeaa7f8..6cbce91 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -381,19 +381,18 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  {
>  	s32 value;
>  	int x;
> -	char ascii_value[11];
> +	char ascii_value[12];
>  	struct pm_qos_request_list *pm_qos_req;
>  
>  	if (count == sizeof(s32)) {
>  		if (copy_from_user(&value, buf, sizeof(s32)))
>  			return -EFAULT;
> -	} else if (count == 11) { /* len('0x12345678/0') */
> -		if (copy_from_user(ascii_value, buf, 11))
> +	} else if (count == 10 || count == 12) { /* '0x12345678' or
	} else if (count == 10 || count == 11) { /* '0x12345678' or
> +						    '0x12345678/n/0'*/
> +		memset(ascii_value, 0, sizeof(ascii_value));
> +		if (copy_from_user(ascii_value, buf, count))
		if (copy_from_user(ascii_value, buf, 10))
						
>  			return -EFAULT;
> -		if (strlen(ascii_value) != 10)
> -			return -EINVAL;
> -		x = sscanf(ascii_value, "%x", &value);
> -		if (x != 1)
> +		if (strict_strtoul(ascii_value,16,value) != 0)
                                                  ^&value
its odd the array ascii_value is getting stomped on.  Very strange.
I'm sure I'm doing something dumb.  I'll keep working on this and send
an patch after it checks out.

--mark

>  			return -EINVAL;
>  		pr_debug("%s, %d, 0x%x\n", ascii_value, x, value);
>  	} else
> 
> ------
> I'll test this tomorrow and do a proper posting (if it works right)
> 
> --mark
> > +						  * len('0x12345678') */
> > +		if (copy_from_user(ascii_value, buf, count))
> >  			return -EFAULT;
> > -		if (strlen(ascii_value) != 10)
> > +		if (strlen(ascii_value) != count)
> >  			return -EINVAL;
> >  		x = sscanf(ascii_value, "%x", &value);
> >  		if (x != 1)
> > -- 
> > 1.7.2.3
> > 

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[Alan Stern]

On Tue, 22 Feb 2011, mark gross wrote:

> On Mon, Feb 21, 2011 at 08:33:36PM -0800, mark gross wrote:
> > On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> > > In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> > > the parsing of the ASCII hex value was tightened. Unfortunately
> > > it was tightened to the point where no value is valid.
> > > 
> > > Root of the problem seems to lie in wheather the ASCII hex is followed
> > > by a '\n' or not. My reading of the documentation is that the '\n' should
> > > not be present. However the code previously only accepted that version.
> > > The current code accepts neither. My fix is to accept both.
> > > 
> > > Cc: Mark Gross <markgross@thegnar.org>
> > > Cc: Dan Carpenter <error27@gmail.com>
> > > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > > 
> > > ---
> > > This appears to have been introduced around 2.6.36-rc4.
> > > And was an @stable patch. As such I believe this change
> > > is stable material.
> > > ---
> > >  kernel/pm_qos_params.c |    7 ++++---
> > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > index aeaa7f8..98a34ea 100644
> > > --- a/kernel/pm_qos_params.c
> > > +++ b/kernel/pm_qos_params.c
> > > @@ -387,10 +387,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> > >  	if (count == sizeof(s32)) {
> > >  		if (copy_from_user(&value, buf, sizeof(s32)))
> > >  			return -EFAULT;
> > > -	} else if (count == 11) { /* len('0x12345678/0') */
> > > -		if (copy_from_user(ascii_value, buf, 11))
> > > +	} else if (count == 11 || count == 10) { /* len('0x12345678\n') or
> > shouldn't this be count ==12 ||count ==10?
> > After taking Alan's advice and looking at strict_strtoul it looks like
> > 10 and 12 are the numbers to use.
> > 
> > Also playing with your dd test:
> > mgross@mgt:~$ echo -n 0x12345678 | dd of=junk.bin
> > 0+1 records in
> > 0+1 records out
> > 10 bytes (10 B) copied, 0.000368621 s, 27.1 kB/s
> > mgross@mgt:~$ hexdump junk.bin 
> > 0000000 7830 3231 3433 3635 3837               
> > 000000a
> > mgross@mgt:~$ echo  0x12345678 | dd of=junk.bin
> > 0+1 records in
> > 0+1 records out
> > 11 bytes (11 B) copied, 0.000384755 s, 28.6 kB/s
> > mgross@mgt:~$ hexdump junk.bin 
> > 0000000 7830 3231 3433 3635 3837 000a          
> > 000000b

The "extra" byte here is undoubtedly caused by the fact that hexdump is 
presenting the data in 2-byte chunks.  I almost always invoke hexdump 
with the -C option, to get individual bytes.  The length of the data 
really is 11 bytes.

> > 
> > it looks like I have 10 or 12 bytes (but can't reconcile the dd output
> > saying 11 bytes when hexdump is showing 12 )
> 
> on a 64 bit ubuntu box its 11 bytes that get sent to the kernel hexdump
> must be padding the extra byte.> 
> > The following patch (untested) should make things work ok.
> it doesn't work and I'm still debugging some straingess with  strict_strtoul
> 
> it looks like its munging the string I'm passing in.

No, strict_strtoul() doesn't touch the input string.

> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index aeaa7f8..6cbce91 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -381,19 +381,18 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> >  {
> >  	s32 value;
> >  	int x;
> > -	char ascii_value[11];
> > +	char ascii_value[12];
> >  	struct pm_qos_request_list *pm_qos_req;
> >  
> >  	if (count == sizeof(s32)) {
> >  		if (copy_from_user(&value, buf, sizeof(s32)))
> >  			return -EFAULT;
> > -	} else if (count == 11) { /* len('0x12345678/0') */
> > -		if (copy_from_user(ascii_value, buf, 11))
> > +	} else if (count == 10 || count == 12) { /* '0x12345678' or
> 	} else if (count == 10 || count == 11) { /* '0x12345678' or

The test should be for 10 or 11: "0x12345678" or "0x12345678\n".  By 
the way, note the difference between "\n" and "/n".

> > +						    '0x12345678/n/0'*/
> > +		memset(ascii_value, 0, sizeof(ascii_value));

Actually, it suffices to do

		ascii_value[count] = 0;

if the number of bytes you copy is equal to count.

> > +		if (copy_from_user(ascii_value, buf, count))
> 		if (copy_from_user(ascii_value, buf, 10))

Which line does the patch add?

> >  			return -EFAULT;
> > -		if (strlen(ascii_value) != 10)
> > -			return -EINVAL;
> > -		x = sscanf(ascii_value, "%x", &value);
> > -		if (x != 1)
> > +		if (strict_strtoul(ascii_value,16,value) != 0)
>                                                   ^&value
> its odd the array ascii_value is getting stomped on.  Very strange.
> I'm sure I'm doing something dumb.  I'll keep working on this and send
> an patch after it checks out.

How careful do you want to be here?  For example, which of the
following inputs do you want to accept?

	0x1234
	abcd1234
	abcd123456
	abcd123456\n
	abcd1234567
	1234567890
	1234567890\n
	12345678901
	0x12345678
	0x12345678\n
	0x123456789

Maybe it's okay to be a little relaxed about this, and trust the caller 
to pass in data that makes sense.

Alan Stern

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[mark gross]

On Wed, Feb 23, 2011 at 10:20:35AM -0500, Alan Stern wrote:
> On Tue, 22 Feb 2011, mark gross wrote:
> 
> > On Mon, Feb 21, 2011 at 08:33:36PM -0800, mark gross wrote:
> > > On Fri, Feb 18, 2011 at 10:54:56AM +0900, Simon Horman wrote:
> > > > In "PM QoS: Correct pr_debug() misuse and improve parameter checks"
> > > > the parsing of the ASCII hex value was tightened. Unfortunately
> > > > it was tightened to the point where no value is valid.
> > > > 
> > > > Root of the problem seems to lie in wheather the ASCII hex is followed
> > > > by a '\n' or not. My reading of the documentation is that the '\n' should
> > > > not be present. However the code previously only accepted that version.
> > > > The current code accepts neither. My fix is to accept both.
> > > > 
> > > > Cc: Mark Gross <markgross@thegnar.org>
> > > > Cc: Dan Carpenter <error27@gmail.com>
> > > > Cc: Rafael J. Wysocki <rjw@sisk.pl>
> > > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > > > 
> > > > ---
> > > > This appears to have been introduced around 2.6.36-rc4.
> > > > And was an @stable patch. As such I believe this change
> > > > is stable material.
> > > > ---
> > > >  kernel/pm_qos_params.c |    7 ++++---
> > > >  1 files changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > > index aeaa7f8..98a34ea 100644
> > > > --- a/kernel/pm_qos_params.c
> > > > +++ b/kernel/pm_qos_params.c
> > > > @@ -387,10 +387,11 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> > > >  	if (count == sizeof(s32)) {
> > > >  		if (copy_from_user(&value, buf, sizeof(s32)))
> > > >  			return -EFAULT;
> > > > -	} else if (count == 11) { /* len('0x12345678/0') */
> > > > -		if (copy_from_user(ascii_value, buf, 11))
> > > > +	} else if (count == 11 || count == 10) { /* len('0x12345678\n') or
> > > shouldn't this be count ==12 ||count ==10?
> > > After taking Alan's advice and looking at strict_strtoul it looks like
> > > 10 and 12 are the numbers to use.
> > > 
> > > Also playing with your dd test:
> > > mgross@mgt:~$ echo -n 0x12345678 | dd of=junk.bin
> > > 0+1 records in
> > > 0+1 records out
> > > 10 bytes (10 B) copied, 0.000368621 s, 27.1 kB/s
> > > mgross@mgt:~$ hexdump junk.bin 
> > > 0000000 7830 3231 3433 3635 3837               
> > > 000000a
> > > mgross@mgt:~$ echo  0x12345678 | dd of=junk.bin
> > > 0+1 records in
> > > 0+1 records out
> > > 11 bytes (11 B) copied, 0.000384755 s, 28.6 kB/s
> > > mgross@mgt:~$ hexdump junk.bin 
> > > 0000000 7830 3231 3433 3635 3837 000a          
> > > 000000b
> 
> The "extra" byte here is undoubtedly caused by the fact that hexdump is 
> presenting the data in 2-byte chunks.  I almost always invoke hexdump 
> with the -C option, to get individual bytes.  The length of the data 
> really is 11 bytes.
> 
> > > 
> > > it looks like I have 10 or 12 bytes (but can't reconcile the dd output
> > > saying 11 bytes when hexdump is showing 12 )
> > 
> > on a 64 bit ubuntu box its 11 bytes that get sent to the kernel hexdump
> > must be padding the extra byte.> 
> > > The following patch (untested) should make things work ok.
> > it doesn't work and I'm still debugging some straingess with  strict_strtoul
> > 
> > it looks like its munging the string I'm passing in.
> 
> No, strict_strtoul() doesn't touch the input string.

I've been having really bad luck with this function.  I must be having a
operator error happening.  I'm still looking at it and plan on setting
up more tracing tonight.
 
> > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > index aeaa7f8..6cbce91 100644
> > > --- a/kernel/pm_qos_params.c
> > > +++ b/kernel/pm_qos_params.c
> > > @@ -381,19 +381,18 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> > >  {
> > >  	s32 value;
> > >  	int x;
> > > -	char ascii_value[11];
> > > +	char ascii_value[12];
> > >  	struct pm_qos_request_list *pm_qos_req;
> > >  
> > >  	if (count == sizeof(s32)) {
> > >  		if (copy_from_user(&value, buf, sizeof(s32)))
> > >  			return -EFAULT;
> > > -	} else if (count == 11) { /* len('0x12345678/0') */
> > > -		if (copy_from_user(ascii_value, buf, 11))
> > > +	} else if (count == 10 || count == 12) { /* '0x12345678' or
> > 	} else if (count == 10 || count == 11) { /* '0x12345678' or
> 
> The test should be for 10 or 11: "0x12345678" or "0x12345678\n".  By 
> the way, note the difference between "\n" and "/n".
> 
> > > +						    '0x12345678/n/0'*/
> > > +		memset(ascii_value, 0, sizeof(ascii_value));
> 
> Actually, it suffices to do
> 
> 		ascii_value[count] = 0;
> 
> if the number of bytes you copy is equal to count.
true.
 
> > > +		if (copy_from_user(ascii_value, buf, count))
> > 		if (copy_from_user(ascii_value, buf, 10))
> 
> Which line does the patch add?
> 
> > >  			return -EFAULT;
> > > -		if (strlen(ascii_value) != 10)
> > > -			return -EINVAL;
> > > -		x = sscanf(ascii_value, "%x", &value);
> > > -		if (x != 1)
> > > +		if (strict_strtoul(ascii_value,16,value) != 0)
> >                                                   ^&value
> > its odd the array ascii_value is getting stomped on.  Very strange.
> > I'm sure I'm doing something dumb.  I'll keep working on this and send
> > an patch after it checks out.
> 
> How careful do you want to be here?  For example, which of the
> following inputs do you want to accept?
> 
> 	0x1234
> 	abcd1234
> 	abcd123456
> 	abcd123456\n
> 	abcd1234567
> 	1234567890
> 	1234567890\n
> 	12345678901

> 	0x12345678
> 	0x12345678\n
just these 2 are what I had planned to allow after this email thread.

> 	0x123456789
> 
> Maybe it's okay to be a little relaxed about this, and trust the caller 
> to pass in data that makes sense.
yeah but is it worth the effort?

--mark
 

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[Alan Stern]

On Thu, 24 Feb 2011, mark gross wrote:

> > How careful do you want to be here?  For example, which of the
> > following inputs do you want to accept?
> > 
> > 	0x1234
> > 	abcd1234
> > 	abcd123456
> > 	abcd123456\n
> > 	abcd1234567
> > 	1234567890
> > 	1234567890\n
> > 	12345678901
> 
> > 	0x12345678
> > 	0x12345678\n
> just these 2 are what I had planned to allow after this email thread.
> 
> > 	0x123456789
> > 
> > Maybe it's okay to be a little relaxed about this, and trust the caller 
> > to pass in data that makes sense.
> yeah but is it worth the effort?

Checking for exactly those two forms really is a lot of effort.  You
have to make sure the first two characters are "0x" or "0X", you have
to check that each of the next eight characters is a valid hex digit,
and you have to verify that the 11th character, if present, is a
newline.

If you can get results that are good enough just by calling
strict_strtoul() without all these checks, it's probably worthwhile.

Alan Stern

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[mark gross]

On Thu, Feb 24, 2011 at 12:00:35PM -0500, Alan Stern wrote:
> On Thu, 24 Feb 2011, mark gross wrote:
> 
> > > How careful do you want to be here?  For example, which of the
> > > following inputs do you want to accept?
> > > 
> > > 	0x1234
> > > 	abcd1234
> > > 	abcd123456
> > > 	abcd123456\n
> > > 	abcd1234567
> > > 	1234567890
> > > 	1234567890\n
> > > 	12345678901
> > 
> > > 	0x12345678
> > > 	0x12345678\n
> > just these 2 are what I had planned to allow after this email thread.
> > 
> > > 	0x123456789
> > > 
> > > Maybe it's okay to be a little relaxed about this, and trust the caller 
> > > to pass in data that makes sense.
> > yeah but is it worth the effort?
> 
> Checking for exactly those two forms really is a lot of effort.  You
> have to make sure the first two characters are "0x" or "0X", you have
> to check that each of the next eight characters is a valid hex digit,
> and you have to verify that the 11th character, if present, is a
> newline.
> 
> If you can get results that are good enough just by calling
> strict_strtoul() without all these checks, it's probably worthwhile.
>
I just don't want any buffer overrun bugs in code I'm writing.

I like the attached thank you for all the really useful input.

--mark
Signed-off-by: mark gross <markgross@thegnar.org>


>From df199e491c750c529abcfb0e2256f508f1afd061 Mon Sep 17 00:00:00 2001
From: mark gross <markgross@thegnar.org>
Date: Sun, 6 Mar 2011 05:45:44 -0800
Subject: [PATCH] correct PM QOS's user mode interface to work with ascii input per
 what is in the kernel docs.  Writing a string to the ABI from user mode
 comes in 2 flavors.  one with and one without a '\n' at the end.  this
 change accepts both.

 echo 0x12345678 > /dev/cpu_dma_latency
and
 echo -n 0x12345678 > /dev/cpu_dma_latency
now both work.
---
 kernel/pm_qos_params.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index aeaa7f8..b315446 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -40,6 +40,7 @@
 #include <linux/string.h>
 #include <linux/platform_device.h>
 #include <linux/init.h>
+#include <linux/kernel.h>
 
 #include <linux/uaccess.h>
 
@@ -387,15 +388,15 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 	if (count == sizeof(s32)) {
 		if (copy_from_user(&value, buf, sizeof(s32)))
 			return -EFAULT;
-	} else if (count == 11) { /* len('0x12345678/0') */
-		if (copy_from_user(ascii_value, buf, 11))
+	} else if (count == 10 || count == 11) { /* '0x12345678' or
+						    '0x12345678/n'*/
+		ascii_value[count] = 0;
+		if (copy_from_user(ascii_value, buf, count))
 			return -EFAULT;
-		if (strlen(ascii_value) != 10)
+		if ((x=strict_strtol(ascii_value, 16, &value)) != 0){
+			pr_debug("%s, 0x%x, 0x%x\n",ascii_value, value, x);
 			return -EINVAL;
-		x = sscanf(ascii_value, "%x", &value);
-		if (x != 1)
-			return -EINVAL;
-		pr_debug("%s, %d, 0x%x\n", ascii_value, x, value);
+		}
 	} else
 		return -EINVAL;
 
-- 
1.7.1

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[Rafael J. Wysocki]

On Sunday, March 06, 2011, mark gross wrote:
> On Thu, Feb 24, 2011 at 12:00:35PM -0500, Alan Stern wrote:
> > On Thu, 24 Feb 2011, mark gross wrote:
> > 
> > > > How careful do you want to be here?  For example, which of the
> > > > following inputs do you want to accept?
> > > > 
> > > > 	0x1234
> > > > 	abcd1234
> > > > 	abcd123456
> > > > 	abcd123456\n
> > > > 	abcd1234567
> > > > 	1234567890
> > > > 	1234567890\n
> > > > 	12345678901
> > > 
> > > > 	0x12345678
> > > > 	0x12345678\n
> > > just these 2 are what I had planned to allow after this email thread.
> > > 
> > > > 	0x123456789
> > > > 
> > > > Maybe it's okay to be a little relaxed about this, and trust the caller 
> > > > to pass in data that makes sense.
> > > yeah but is it worth the effort?
> > 
> > Checking for exactly those two forms really is a lot of effort.  You
> > have to make sure the first two characters are "0x" or "0X", you have
> > to check that each of the next eight characters is a valid hex digit,
> > and you have to verify that the 11th character, if present, is a
> > newline.
> > 
> > If you can get results that are good enough just by calling
> > strict_strtoul() without all these checks, it's probably worthwhile.
> >
> I just don't want any buffer overrun bugs in code I'm writing.
> 
> I like the attached thank you for all the really useful input.
> 
> --mark
> Signed-off-by: mark gross <markgross@thegnar.org>

Mark, do you want the patch below to be merged?

Rafael


> From df199e491c750c529abcfb0e2256f508f1afd061 Mon Sep 17 00:00:00 2001
> From: mark gross <markgross@thegnar.org>
> Date: Sun, 6 Mar 2011 05:45:44 -0800
> Subject: [PATCH] correct PM QOS's user mode interface to work with ascii input per
>  what is in the kernel docs.  Writing a string to the ABI from user mode
>  comes in 2 flavors.  one with and one without a '\n' at the end.  this
>  change accepts both.
> 
>  echo 0x12345678 > /dev/cpu_dma_latency
> and
>  echo -n 0x12345678 > /dev/cpu_dma_latency
> now both work.
> ---
>  kernel/pm_qos_params.c |   15 ++++++++-------
>  1 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index aeaa7f8..b315446 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -40,6 +40,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/init.h>
> +#include <linux/kernel.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -387,15 +388,15 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
>  	if (count == sizeof(s32)) {
>  		if (copy_from_user(&value, buf, sizeof(s32)))
>  			return -EFAULT;
> -	} else if (count == 11) { /* len('0x12345678/0') */
> -		if (copy_from_user(ascii_value, buf, 11))
> +	} else if (count == 10 || count == 11) { /* '0x12345678' or
> +						    '0x12345678/n'*/
> +		ascii_value[count] = 0;
> +		if (copy_from_user(ascii_value, buf, count))
>  			return -EFAULT;
> -		if (strlen(ascii_value) != 10)
> +		if ((x=strict_strtol(ascii_value, 16, &value)) != 0){
> +			pr_debug("%s, 0x%x, 0x%x\n",ascii_value, value, x);
>  			return -EINVAL;
> -		x = sscanf(ascii_value, "%x", &value);
> -		if (x != 1)
> -			return -EINVAL;
> -		pr_debug("%s, %d, 0x%x\n", ascii_value, x, value);
> +		}
>  	} else
>  		return -EINVAL;
>  
> 

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[mark gross]

On Tue, Mar 29, 2011 at 10:01:24PM +0200, Rafael J. Wysocki wrote:
> On Sunday, March 06, 2011, mark gross wrote:
> > On Thu, Feb 24, 2011 at 12:00:35PM -0500, Alan Stern wrote:
> > > On Thu, 24 Feb 2011, mark gross wrote:
> > > 
> > > > > How careful do you want to be here?  For example, which of the
> > > > > following inputs do you want to accept?
> > > > > 
> > > > > 	0x1234
> > > > > 	abcd1234
> > > > > 	abcd123456
> > > > > 	abcd123456\n
> > > > > 	abcd1234567
> > > > > 	1234567890
> > > > > 	1234567890\n
> > > > > 	12345678901
> > > > 
> > > > > 	0x12345678
> > > > > 	0x12345678\n
> > > > just these 2 are what I had planned to allow after this email thread.
> > > > 
> > > > > 	0x123456789
> > > > > 
> > > > > Maybe it's okay to be a little relaxed about this, and trust the caller 
> > > > > to pass in data that makes sense.
> > > > yeah but is it worth the effort?
> > > 
> > > Checking for exactly those two forms really is a lot of effort.  You
> > > have to make sure the first two characters are "0x" or "0X", you have
> > > to check that each of the next eight characters is a valid hex digit,
> > > and you have to verify that the 11th character, if present, is a
> > > newline.
> > > 
> > > If you can get results that are good enough just by calling
> > > strict_strtoul() without all these checks, it's probably worthwhile.
> > >
> > I just don't want any buffer overrun bugs in code I'm writing.
> > 
> > I like the attached thank you for all the really useful input.
> > 
> > --mark
> > Signed-off-by: mark gross <markgross@thegnar.org>
> 
> Mark, do you want the patch below to be merged?
> 
> Rafael
> 

Yes.  There is was some discussion on if we should relax the value
checking beyond this that died off.  But, at a minimum I think this
should be merged as it fixes the usability issue raised by Simon.

thanks,

--mark

> 
> > From df199e491c750c529abcfb0e2256f508f1afd061 Mon Sep 17 00:00:00 2001
> > From: mark gross <markgross@thegnar.org>
> > Date: Sun, 6 Mar 2011 05:45:44 -0800
> > Subject: [PATCH] correct PM QOS's user mode interface to work with ascii input per
> >  what is in the kernel docs.  Writing a string to the ABI from user mode
> >  comes in 2 flavors.  one with and one without a '\n' at the end.  this
> >  change accepts both.
> > 
> >  echo 0x12345678 > /dev/cpu_dma_latency
> > and
> >  echo -n 0x12345678 > /dev/cpu_dma_latency
> > now both work.
> > ---
> >  kernel/pm_qos_params.c |   15 ++++++++-------
> >  1 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index aeaa7f8..b315446 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -40,6 +40,7 @@
> >  #include <linux/string.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/init.h>
> > +#include <linux/kernel.h>
> >  
> >  #include <linux/uaccess.h>
> >  
> > @@ -387,15 +388,15 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> >  	if (count == sizeof(s32)) {
> >  		if (copy_from_user(&value, buf, sizeof(s32)))
> >  			return -EFAULT;
> > -	} else if (count == 11) { /* len('0x12345678/0') */
> > -		if (copy_from_user(ascii_value, buf, 11))
> > +	} else if (count == 10 || count == 11) { /* '0x12345678' or
> > +						    '0x12345678/n'*/
> > +		ascii_value[count] = 0;
> > +		if (copy_from_user(ascii_value, buf, count))
> >  			return -EFAULT;
> > -		if (strlen(ascii_value) != 10)
> > +		if ((x=strict_strtol(ascii_value, 16, &value)) != 0){
> > +			pr_debug("%s, 0x%x, 0x%x\n",ascii_value, value, x);
> >  			return -EINVAL;
> > -		x = sscanf(ascii_value, "%x", &value);
> > -		if (x != 1)
> > -			return -EINVAL;
> > -		pr_debug("%s, %d, 0x%x\n", ascii_value, x, value);
> > +		}
> >  	} else
> >  		return -EINVAL;
> >  
> > 
> 

Re: [PATCH] PM QoS: Allow parsing of ASCII values

[Simon Horman]

On Tue, Mar 29, 2011 at 08:59:11PM -0700, mark gross wrote:
> On Tue, Mar 29, 2011 at 10:01:24PM +0200, Rafael J. Wysocki wrote:
> > On Sunday, March 06, 2011, mark gross wrote:
> > > On Thu, Feb 24, 2011 at 12:00:35PM -0500, Alan Stern wrote:
> > > > On Thu, 24 Feb 2011, mark gross wrote:
> > > > 
> > > > > > How careful do you want to be here?  For example, which of the
> > > > > > following inputs do you want to accept?
> > > > > > 
> > > > > > 	0x1234
> > > > > > 	abcd1234
> > > > > > 	abcd123456
> > > > > > 	abcd123456\n
> > > > > > 	abcd1234567
> > > > > > 	1234567890
> > > > > > 	1234567890\n
> > > > > > 	12345678901
> > > > > 
> > > > > > 	0x12345678
> > > > > > 	0x12345678\n
> > > > > just these 2 are what I had planned to allow after this email thread.
> > > > > 
> > > > > > 	0x123456789
> > > > > > 
> > > > > > Maybe it's okay to be a little relaxed about this, and trust the caller 
> > > > > > to pass in data that makes sense.
> > > > > yeah but is it worth the effort?
> > > > 
> > > > Checking for exactly those two forms really is a lot of effort.  You
> > > > have to make sure the first two characters are "0x" or "0X", you have
> > > > to check that each of the next eight characters is a valid hex digit,
> > > > and you have to verify that the 11th character, if present, is a
> > > > newline.
> > > > 
> > > > If you can get results that are good enough just by calling
> > > > strict_strtoul() without all these checks, it's probably worthwhile.
> > > >
> > > I just don't want any buffer overrun bugs in code I'm writing.
> > > 
> > > I like the attached thank you for all the really useful input.
> > > 
> > > --mark
> > > Signed-off-by: mark gross <markgross@thegnar.org>
> > 
> > Mark, do you want the patch below to be merged?
> > 
> > Rafael
> > 
> 
> Yes.  There is was some discussion on if we should relax the value
> checking beyond this that died off.  But, at a minimum I think this
> should be merged as it fixes the usability issue raised by Simon.

Not entirely surprisingly, I agree :-)

Acked-by: Simon Horman <horms@verge.net.au>
> 
> thanks,
> 
> --mark
> 
> > 
> > > From df199e491c750c529abcfb0e2256f508f1afd061 Mon Sep 17 00:00:00 2001
> > > From: mark gross <markgross@thegnar.org>
> > > Date: Sun, 6 Mar 2011 05:45:44 -0800
> > > Subject: [PATCH] correct PM QOS's user mode interface to work with ascii input per
> > >  what is in the kernel docs.  Writing a string to the ABI from user mode
> > >  comes in 2 flavors.  one with and one without a '\n' at the end.  this
> > >  change accepts both.
> > > 
> > >  echo 0x12345678 > /dev/cpu_dma_latency
> > > and
> > >  echo -n 0x12345678 > /dev/cpu_dma_latency
> > > now both work.
> > > ---
> > >  kernel/pm_qos_params.c |   15 ++++++++-------
> > >  1 files changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > index aeaa7f8..b315446 100644
> > > --- a/kernel/pm_qos_params.c
> > > +++ b/kernel/pm_qos_params.c
> > > @@ -40,6 +40,7 @@
> > >  #include <linux/string.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/init.h>
> > > +#include <linux/kernel.h>
> > >  
> > >  #include <linux/uaccess.h>
> > >  
> > > @@ -387,15 +388,15 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
> > >  	if (count == sizeof(s32)) {
> > >  		if (copy_from_user(&value, buf, sizeof(s32)))
> > >  			return -EFAULT;
> > > -	} else if (count == 11) { /* len('0x12345678/0') */
> > > -		if (copy_from_user(ascii_value, buf, 11))
> > > +	} else if (count == 10 || count == 11) { /* '0x12345678' or
> > > +						    '0x12345678/n'*/
> > > +		ascii_value[count] = 0;
> > > +		if (copy_from_user(ascii_value, buf, count))
> > >  			return -EFAULT;
> > > -		if (strlen(ascii_value) != 10)
> > > +		if ((x=strict_strtol(ascii_value, 16, &value)) != 0){
> > > +			pr_debug("%s, 0x%x, 0x%x\n",ascii_value, value, x);
> > >  			return -EINVAL;
> > > -		x = sscanf(ascii_value, "%x", &value);
> > > -		if (x != 1)
> > > -			return -EINVAL;
> > > -		pr_debug("%s, %d, 0x%x\n", ascii_value, x, value);
> > > +		}
> > >  	} else
> > >  		return -EINVAL;
> > >  
> > > 
> > 
>