[PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 .../lustre/lustre/obdclass/lprocfs_status.c        |   24 +++++++++++---------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 61e04af..5227324 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
 	}
 
 	units = 1;
-	switch (*end) {
-	case 'p': case 'P':
-		units <<= 10;
-	case 't': case 'T':
-		units <<= 10;
-	case 'g': case 'G':
-		units <<= 10;
-	case 'm': case 'M':
-		units <<= 10;
-	case 'k': case 'K':
-		units <<= 10;
+	if (end) {
+		switch (*end) {
+		case 'p': case 'P':
+			units <<= 10;
+		case 't': case 'T':
+			units <<= 10;
+		case 'g': case 'G':
+			units <<= 10;
+		case 'm': case 'M':
+			units <<= 10;
+		case 'k': case 'K':
+			units <<= 10;
+		}
 	}
 	/* Specified units override the multiplier */
 	if (units)
-- 
1.7.10.4

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Joe Perches]

On Sun, 2014-12-14 at 23:52 +0100, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.

Perhaps the tool could use a little work.
It's not possible for end to be NULL no?

unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
{
	unsigned long long result;
	unsigned int rv;

	cp = _parse_integer_fixup_radix(cp, &base);
	rv = _parse_integer(cp, base, &result);
	/* FIXME */
	cp += (rv & ~KSTRTOX_OVERFLOW);

	if (endp)
		*endp = (char *)cp;

	return result;
}
EXPORT_SYMBOL(simple_strtoull);


> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
[]

Above this:

	whole = simple_strtoull(pbuf, &end, 10);

> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
>  	}
>  
>  	units = 1;
> -	switch (*end) {
> -	case 'p': case 'P':
> -		units <<= 10;
> -	case 't': case 'T':
> -		units <<= 10;
> -	case 'g': case 'G':
> -		units <<= 10;
> -	case 'm': case 'M':
> -		units <<= 10;
> -	case 'k': case 'K':
> -		units <<= 10;
> +	if (end) {
> +		switch (*end) {
> +		case 'p': case 'P':
> +			units <<= 10;
> +		case 't': case 'T':
> +			units <<= 10;
> +		case 'g': case 'G':
> +			units <<= 10;
> +		case 'm': case 'M':
> +			units <<= 10;
> +		case 'k': case 'K':
> +			units <<= 10;
> +		}

The only thing I might do is

	switch (tolower(*end)) {

and remove the second case entry for each line

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

Hi Joe

No, it does not look like end can be NULL then.
Then remove the end != NULL instead?
...
    if (end != NULL && *end == '.') {


However, I am hesitant to the tolower()  I think double case is faster...?


Kind regards
Rickard Strandqvist


2014-12-15 2:51 GMT+01:00 Joe Perches <joe@perches.com>:
> On Sun, 2014-12-14 at 23:52 +0100, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>
> Perhaps the tool could use a little work.
> It's not possible for end to be NULL no?
>
> unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
> {
>         unsigned long long result;
>         unsigned int rv;
>
>         cp = _parse_integer_fixup_radix(cp, &base);
>         rv = _parse_integer(cp, base, &result);
>         /* FIXME */
>         cp += (rv & ~KSTRTOX_OVERFLOW);
>
>         if (endp)
>                 *endp = (char *)cp;
>
>         return result;
> }
> EXPORT_SYMBOL(simple_strtoull);
>
>
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> []
>
> Above this:
>
>         whole = simple_strtoull(pbuf, &end, 10);
>
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -1897,17 +1897,19 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
>>       }
>>
>>       units = 1;
>> -     switch (*end) {
>> -     case 'p': case 'P':
>> -             units <<= 10;
>> -     case 't': case 'T':
>> -             units <<= 10;
>> -     case 'g': case 'G':
>> -             units <<= 10;
>> -     case 'm': case 'M':
>> -             units <<= 10;
>> -     case 'k': case 'K':
>> -             units <<= 10;
>> +     if (end) {
>> +             switch (*end) {
>> +             case 'p': case 'P':
>> +                     units <<= 10;
>> +             case 't': case 'T':
>> +                     units <<= 10;
>> +             case 'g': case 'G':
>> +                     units <<= 10;
>> +             case 'm': case 'M':
>> +                     units <<= 10;
>> +             case 'k': case 'K':
>> +                     units <<= 10;
>> +             }
>
> The only thing I might do is
>
>         switch (tolower(*end)) {
>
> and remove the second case entry for each line
>

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Joe Perches]

On Mon, 2014-12-15 at 23:23 +0100, Rickard Strandqvist wrote:
> Hi Joe

Hello Rickard

> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
>     if (end != NULL && *end == '.') {

Up to you.

> However, I am hesitant to the tolower()  I think double case is faster...?

I doubt code execution speed is paramount here.
Maybe see if the object code size is smaller one
way or the other.

RE: [HPDD-discuss] [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Patrick Farrell]

Strongly agreed that execution speed is not critical here.  It's the update of a proc variable, not a tight loop or critical section.

Normally I'd leave it alone, but since you're writing a patch anyway, I'd do 'tolower' myself.  I dislike the stacked case statements on a single line like that.  (It's the only time I've seen them written that way.  Perhaps it's common and I've just missed it.)

Regards,
- Patrick
________________________________________
From: HPDD-discuss [hpdd-discuss-bounces@lists.01.org] on behalf of Joe Perches [joe@perches.com]
Sent: Monday, December 15, 2014 5:53 PM
To: Rickard Strandqvist
Cc: devel@driverdev.osuosl.org; Fabian Frederick; Julia Lawall; James Simmons; Greg Kroah-Hartman; linux-kernel@vger.kernel.org; Greg Donald; HPDD-discuss@lists.01.org; Andriy Skulysh
Subject: Re: [HPDD-discuss] [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

On Mon, 2014-12-15 at 23:23 +0100, Rickard Strandqvist wrote:
> Hi Joe

Hello Rickard

> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
>     if (end != NULL && *end == '.') {

Up to you.

> However, I am hesitant to the tolower()  I think double case is faster...?

I doubt code execution speed is paramount here.
Maybe see if the object code size is smaller one
way or the other.


_______________________________________________
HPDD-discuss mailing list
HPDD-discuss@lists.01.org
https://lists.01.org/mailman/listinfo/hpdd-discuss

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Chris Rorvick]

On Sun, Dec 14, 2014 at 4:52 PM, Rickard Strandqvist
<rickard_strandqvist@spectrumdigital.se> wrote:
>         units = 1;
...
>         /* Specified units override the multiplier */
>         if (units)

That doesn't make much sense.  The conditional logic will always be
executed.  Maybe this is what's intended?

        /* Specified units override the multiplier */
-       if (units)
+       if (units > 1)
                mult = mult < 0 ? -units : units;

Chris

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Dan Carpenter]

On Mon, Dec 15, 2014 at 11:23:25PM +0100, Rickard Strandqvist wrote:
> Hi Joe
> 
> No, it does not look like end can be NULL then.
> Then remove the end != NULL instead?
> ...
>     if (end != NULL && *end == '.') {
> 

Yes.  Please do.

regards,
dan carpenter

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

Sorry! extremely stupid. Sending new patch immediately.

Kind regards
Rickard Strandqvist


2014-12-14 23:39 GMT+01:00 Greg Kroah-Hartman <gregkh@linuxfoundation.org>:
> On Sun, Dec 14, 2014 at 11:36:22PM +0100, Rickard Strandqvist wrote:
>> There is otherwise a risk of a possible null pointer dereference.
>>
>> Was largely found by using a static code analysis program called cppcheck.
>>
>> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
>> ---
>>  .../lustre/lustre/obdclass/lprocfs_status.c        |   20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> index 61e04af..4a7891a 100644
>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
>> @@ -1897,17 +1897,15 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
>>       }
>>
>>       units = 1;
>> -     switch (*end) {
>> -     case 'p': case 'P':
>> -             units <<= 10;
>> -     case 't': case 'T':
>> -             units <<= 10;
>> -     case 'g': case 'G':
>> -             units <<= 10;
>> -     case 'm': case 'M':
>> -             units <<= 10;
>> -     case 'k': case 'K':
>> -             units <<= 10;
>> +     if (end) {
>> +             switch (*end) {
>> +             case 'p': case 'P':
>> +             case 't': case 'T':
>> +             case 'g': case 'G':
>> +             case 'm': case 'M':
>> +             case 'k': case 'K':
>> +                     units <<= 10;
>> +             }
>
> You know you just changed the logic in the code, right?
>
> Why?  Have you tested this?
>
> greg k-h

Re: [PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Greg Kroah-Hartman]

On Sun, Dec 14, 2014 at 11:36:22PM +0100, Rickard Strandqvist wrote:
> There is otherwise a risk of a possible null pointer dereference.
> 
> Was largely found by using a static code analysis program called cppcheck.
> 
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
> ---
>  .../lustre/lustre/obdclass/lprocfs_status.c        |   20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> index 61e04af..4a7891a 100644
> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
> @@ -1897,17 +1897,15 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
>  	}
>  
>  	units = 1;
> -	switch (*end) {
> -	case 'p': case 'P':
> -		units <<= 10;
> -	case 't': case 'T':
> -		units <<= 10;
> -	case 'g': case 'G':
> -		units <<= 10;
> -	case 'm': case 'M':
> -		units <<= 10;
> -	case 'k': case 'K':
> -		units <<= 10;
> +	if (end) {
> +		switch (*end) {
> +		case 'p': case 'P':
> +		case 't': case 'T':
> +		case 'g': case 'G':
> +		case 'm': case 'M':
> +		case 'k': case 'K':
> +			units <<= 10;
> +		}

You know you just changed the logic in the code, right?

Why?  Have you tested this?

greg k-h

[PATCH] staging: lustre: lustre: obdclass: lprocfs_status.c: Fix for possible null pointer dereference

[Rickard Strandqvist]

There is otherwise a risk of a possible null pointer dereference.

Was largely found by using a static code analysis program called cppcheck.

Signed-off-by: Rickard Strandqvist <rickard_strandqvist@spectrumdigital.se>
---
 .../lustre/lustre/obdclass/lprocfs_status.c        |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
index 61e04af..4a7891a 100644
--- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
+++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c
@@ -1897,17 +1897,15 @@ int lprocfs_write_frac_u64_helper(const char *buffer, unsigned long count,
 	}
 
 	units = 1;
-	switch (*end) {
-	case 'p': case 'P':
-		units <<= 10;
-	case 't': case 'T':
-		units <<= 10;
-	case 'g': case 'G':
-		units <<= 10;
-	case 'm': case 'M':
-		units <<= 10;
-	case 'k': case 'K':
-		units <<= 10;
+	if (end) {
+		switch (*end) {
+		case 'p': case 'P':
+		case 't': case 'T':
+		case 'g': case 'G':
+		case 'm': case 'M':
+		case 'k': case 'K':
+			units <<= 10;
+		}
 	}
 	/* Specified units override the multiplier */
 	if (units)
-- 
1.7.10.4