2012-11-17 02:00:08

by Cyril Roelandt

Subject: [PATCH] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

The LEN variable is unsigned, therefore checking whether it is less than 0 is
useless.

Signed-off-by: Cyril Roelandt <[email protected]>
---
drivers/acpi/proc.c | 2 --
1 file changed, 2 deletions(-)

diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
index 27adb09..37871a7 100644
--- a/drivers/acpi/proc.c
+++ b/drivers/acpi/proc.c
@@ -366,8 +366,6 @@ acpi_system_write_wakeup_device(struct file *file,

if (len > 4)
len = 4;
- if (len < 0)
- return -EFAULT;

if (copy_from_user(strbuf, buffer, len))
return -EFAULT;
--
1.7.10.4

2012-11-21 00:40:34

by Rafael J. Wysocki

Subject: Re: [PATCH] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

On Saturday, November 17, 2012 02:54:23 AM Cyril Roelandt wrote:
> The LEN variable is unsigned, therefore checking whether it is less than 0 is
> useless.
>
> Signed-off-by: Cyril Roelandt <[email protected]>
> ---
> drivers/acpi/proc.c | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
> index 27adb09..37871a7 100644
> --- a/drivers/acpi/proc.c
> +++ b/drivers/acpi/proc.c
> @@ -366,8 +366,6 @@ acpi_system_write_wakeup_device(struct file *file,
>
> if (len > 4)
> len = 4;
> - if (len < 0)
> - return -EFAULT;

I would prefer to check if count < 0 instead.

>
> if (copy_from_user(strbuf, buffer, len))
> return -EFAULT;

Thanks,
Rafael


--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2012-11-22 19:15:36

by Cyril Roelandt

Subject: [PATCH v2] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

The LEN variable is unsigned, therefore checking whether it is less than 0 is
useless. Also drop the LEN variable, since the COUNT parameter can be used
instead.

Signed-off-by: Cyril Roelandt <[email protected]>
---
drivers/acpi/proc.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
index 27adb09..ef98796 100644
--- a/drivers/acpi/proc.c
+++ b/drivers/acpi/proc.c
@@ -362,16 +362,13 @@ acpi_system_write_wakeup_device(struct file *file,
struct list_head *node, *next;
char strbuf[5];
char str[5] = "";
- unsigned int len = count;

- if (len > 4)
- len = 4;
- if (len < 0)
- return -EFAULT;
+ if (count > 4)
+ count = 4;

- if (copy_from_user(strbuf, buffer, len))
+ if (copy_from_user(strbuf, buffer, count))
return -EFAULT;
- strbuf[len] = '\0';
+ strbuf[count] = '\0';
sscanf(strbuf, "%s", str);

mutex_lock(&acpi_device_lock);
--
1.7.10.4

2012-11-22 19:20:06

by Cyril Roelandt

Subject: Re: [PATCH] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

On 11/22/2012 01:23 AM, Rafael J. Wysocki wrote:
> On Thursday, November 22, 2012 01:05:30 AM Cyril Roelandt wrote:
>> On 11/21/2012 01:44 AM, Rafael J. Wysocki wrote:
>>> On Saturday, November 17, 2012 02:54:23 AM Cyril Roelandt wrote:
>>>> The LEN variable is unsigned, therefore checking whether it is less than 0 is
>>>> useless.
>>>>
>>>> Signed-off-by: Cyril Roelandt<[email protected]>
>>>> ---
>>>> drivers/acpi/proc.c | 2 --
>>>> 1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
>>>> index 27adb09..37871a7 100644
>>>> --- a/drivers/acpi/proc.c
>>>> +++ b/drivers/acpi/proc.c
>>>> @@ -366,8 +366,6 @@ acpi_system_write_wakeup_device(struct file *file,
>>>>
>>>> if (len> 4)
>>>> len = 4;
>>>> - if (len< 0)
>>>> - return -EFAULT;
>>>
>>> I would prefer to check if count< 0 instead.
>>>
>>
>>
>> count has type size_t, so it is also an unsigned variable. I think that
>> acpi_system_write_wakeup_device really cannot be called with a "count"
>> parameter that is less than 0.
>
> OK
>
> So I suppose we can just drop len entirely and replace it with count, right?
> [And drop the< 0 check too.]
>

Indeed, I'll resend.

Cyril Roelandt.

2012-11-22 19:46:54

by Rafael J. Wysocki

Subject: Re: [PATCH] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

On Thursday, November 22, 2012 01:05:30 AM Cyril Roelandt wrote:
> On 11/21/2012 01:44 AM, Rafael J. Wysocki wrote:
> > On Saturday, November 17, 2012 02:54:23 AM Cyril Roelandt wrote:
> >> The LEN variable is unsigned, therefore checking whether it is less than 0 is
> >> useless.
> >>
> >> Signed-off-by: Cyril Roelandt<[email protected]>
> >> ---
> >> drivers/acpi/proc.c | 2 --
> >> 1 file changed, 2 deletions(-)
> >>
> >> diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
> >> index 27adb09..37871a7 100644
> >> --- a/drivers/acpi/proc.c
> >> +++ b/drivers/acpi/proc.c
> >> @@ -366,8 +366,6 @@ acpi_system_write_wakeup_device(struct file *file,
> >>
> >> if (len> 4)
> >> len = 4;
> >> - if (len< 0)
> >> - return -EFAULT;
> >
> > I would prefer to check if count< 0 instead.
> >
>
>
> count has type size_t, so it is also an unsigned variable. I think that
> acpi_system_write_wakeup_device really cannot be called with a "count"
> parameter that is less than 0.

OK

So I suppose we can just drop len entirely and replace it with count, right?
[And drop the < 0 check too.]

Rafael


--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

2012-11-22 18:42:23

by Cyril Roelandt

Subject: Re: [PATCH] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

On 11/21/2012 01:44 AM, Rafael J. Wysocki wrote:
> On Saturday, November 17, 2012 02:54:23 AM Cyril Roelandt wrote:
>> The LEN variable is unsigned, therefore checking whether it is less than 0 is
>> useless.
>>
>> Signed-off-by: Cyril Roelandt<[email protected]>
>> ---
>> drivers/acpi/proc.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
>> index 27adb09..37871a7 100644
>> --- a/drivers/acpi/proc.c
>> +++ b/drivers/acpi/proc.c
>> @@ -366,8 +366,6 @@ acpi_system_write_wakeup_device(struct file *file,
>>
>> if (len> 4)
>> len = 4;
>> - if (len< 0)
>> - return -EFAULT;
>
> I would prefer to check if count< 0 instead.
>


count has type size_t, so it is also an unsigned variable. I think that
acpi_system_write_wakeup_device really cannot be called with a "count"
parameter that is less than 0.

WBR,
Cyril Roelandt.

2012-11-22 22:35:59

by Rafael J. Wysocki

Subject: Re: [PATCH v2] acpi_system_write_wakeup_device(): fix error check for unsigned variable.

On Thursday, November 22, 2012 01:34:55 AM Cyril Roelandt wrote:
> The LEN variable is unsigned, therefore checking whether it is less than 0 is
> useless. Also drop the LEN variable, since the COUNT parameter can be used
> instead.

Applied (under a different subject).

Thanks,
Rafael


> Signed-off-by: Cyril Roelandt <[email protected]>
> ---
> drivers/acpi/proc.c | 11 ++++-------
> 1 file changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/acpi/proc.c b/drivers/acpi/proc.c
> index 27adb09..ef98796 100644
> --- a/drivers/acpi/proc.c
> +++ b/drivers/acpi/proc.c
> @@ -362,16 +362,13 @@ acpi_system_write_wakeup_device(struct file *file,
> struct list_head *node, *next;
> char strbuf[5];
> char str[5] = "";
> - unsigned int len = count;
>
> - if (len > 4)
> - len = 4;
> - if (len < 0)
> - return -EFAULT;
> + if (count > 4)
> + count = 4;
>
> - if (copy_from_user(strbuf, buffer, len))
> + if (copy_from_user(strbuf, buffer, count))
> return -EFAULT;
> - strbuf[len] = '\0';
> + strbuf[count] = '\0';
> sscanf(strbuf, "%s", str);
>
> mutex_lock(&acpi_device_lock);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.