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@xxxxxxxxxxx>
> > > Cc: Dan Carpenter <error27@xxxxxxxxx>
> > > Cc: Rafael J. Wysocki <rjw@xxxxxxx>
> > > Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>
> > >
> > > ---
> > > 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
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
