[PATCH 1/2] leds: mlxreg: Fix possible buffer overflow

[PATCH 1/2] leds: mlxreg: Fix possible buffer overflow

[Oleh Kravchenko]

Error was detected by PVS-Studio:
V512 A call of the 'sprintf' function will lead to overflow of
the buffer 'led_data->led_cdev_name'.

Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---
 drivers/leds/leds-mlxreg.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/leds/leds-mlxreg.c b/drivers/leds/leds-mlxreg.c
index cabe379071a7..82aea1cd0c12 100644
--- a/drivers/leds/leds-mlxreg.c
+++ b/drivers/leds/leds-mlxreg.c
@@ -228,8 +228,8 @@ static int mlxreg_led_config(struct mlxreg_led_priv_data *priv)
 			brightness = LED_OFF;
 			led_data->base_color = MLXREG_LED_GREEN_SOLID;
 		}
-		sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
-			data->label);
+		snprintf(led_data->led_cdev_name, sizeof(led_data->led_cdev_name),
+			 "mlxreg:%s", data->label);
 		led_cdev->name = led_data->led_cdev_name;
 		led_cdev->brightness = brightness;
 		led_cdev->max_brightness = LED_ON;
-- 
2.21.0

[PATCH 2/2] leds: ns2: Fix wrong boolean expression

[Oleh Kravchenko]

Error was detected by PVS-Studio:
V792 The '__gpio_cansleep' function located to the right of
the operator '|' will be called regardless of the value of
the left operand. Perhaps, it is better to use '||'.

Signed-off-by: Oleh Kravchenko <oleg@kaa.org.ua>
---
 drivers/leds/leds-ns2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/leds/leds-ns2.c b/drivers/leds/leds-ns2.c
index 7c500dfdcfa3..8ae60133a15e 100644
--- a/drivers/leds/leds-ns2.c
+++ b/drivers/leds/leds-ns2.c
@@ -205,7 +205,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
 	led_dat->cdev.groups = ns2_led_groups;
 	led_dat->cmd = template->cmd;
 	led_dat->slow = template->slow;
-	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
+	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) &&
 				gpio_cansleep(led_dat->slow);
 	if (led_dat->can_sleep)
 		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
-- 
2.21.0

Re: [PATCH 2/2] leds: ns2: Fix wrong boolean expression

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Tue 2019-09-03 15:50:20, Oleh Kravchenko wrote:
> Error was detected by PVS-Studio:
> V792 The '__gpio_cansleep' function located to the right of
> the operator '|' will be called regardless of the value of
> the left operand. Perhaps, it is better to use '||'.

1st: original code is not wrong

2nd: you are introducing a bug


> @@ -205,7 +205,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
>  	led_dat->cdev.groups = ns2_led_groups;
>  	led_dat->cmd = template->cmd;
>  	led_dat->slow = template->slow;
> -	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
> +	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) &&
>  				gpio_cansleep(led_dat->slow);
>  	if (led_dat->can_sleep)
>  		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] leds: ns2: Fix wrong boolean expression

[Oleh Kravchenko]

Hello Pavel,

03.09.19 17:12, Pavel Machek пише:
> On Tue 2019-09-03 15:50:20, Oleh Kravchenko wrote:
>> Error was detected by PVS-Studio:
>> V792 The '__gpio_cansleep' function located to the right of
>> the operator '|' will be called regardless of the value of
>> the left operand. Perhaps, it is better to use '||'.
> 1st: original code is not wrong

'|' is bitwise operation, if it really means *OR* then should be '||' -
led_dat->can_sleep is bool.

>
> 2nd: you are introducing a bug

No, because if GPIOs *slow* can sleep and GPIO *cmd* can't sleep
it will call gpio_set_value_cansleep() for both.

>
>
>> @@ -205,7 +205,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
>>  	led_dat->cdev.groups = ns2_led_groups;
>>  	led_dat->cmd = template->cmd;
>>  	led_dat->slow = template->slow;
>> -	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
>> +	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) &&
>>  				gpio_cansleep(led_dat->slow);
>>  	if (led_dat->can_sleep)
>>  		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;

Re: [PATCH 2/2] leds: ns2: Fix wrong boolean expression

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1523 bytes --]

On Tue 2019-09-03 19:00:21, Oleh Kravchenko wrote:
> Hello Pavel,
> 
> 03.09.19 17:12, Pavel Machek пише:
> > On Tue 2019-09-03 15:50:20, Oleh Kravchenko wrote:
> >> Error was detected by PVS-Studio:
> >> V792 The '__gpio_cansleep' function located to the right of
> >> the operator '|' will be called regardless of the value of
> >> the left operand. Perhaps, it is better to use '||'.
> > 1st: original code is not wrong
> 
> '|' is bitwise operation, if it really means *OR* then should be '||' -
> led_dat->can_sleep is bool.

I see that || would be more natural. But | also works.

> > 2nd: you are introducing a bug
> 
> No, because if GPIOs *slow* can sleep and GPIO *cmd* can't sleep
> it will call gpio_set_value_cansleep() for both.

If just one of them can sleep, can_sleep will be 0, and bad things
will happen, right?


> >> @@ -205,7 +205,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
> >>  	led_dat->cdev.groups = ns2_led_groups;
> >>  	led_dat->cmd = template->cmd;
> >>  	led_dat->slow = template->slow;
> >> -	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
> >> +	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) &&
> >>  				gpio_cansleep(led_dat->slow);
> >>  	if (led_dat->can_sleep)
> >>  		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 2/2] leds: ns2: Fix wrong boolean expression

[Oleh Kravchenko]

Pavel,

> On Sep 3, 2019, at 9:50 PM, Pavel Machek <pavel@ucw.cz> wrote:
> 
> On Tue 2019-09-03 19:00:21, Oleh Kravchenko wrote:
>> Hello Pavel,
>> 
>> 03.09.19 17:12, Pavel Machek пише:
>>> On Tue 2019-09-03 15:50:20, Oleh Kravchenko wrote:
>>>> Error was detected by PVS-Studio:
>>>> V792 The '__gpio_cansleep' function located to the right of
>>>> the operator '|' will be called regardless of the value of
>>>> the left operand. Perhaps, it is better to use '||'.
>>> 1st: original code is not wrong
>> 
>> '|' is bitwise operation, if it really means *OR* then should be '||' -
>> led_dat->can_sleep is bool.
> 
> I see that || would be more natural. But | also works.
> 
>>> 2nd: you are introducing a bug
>> 
>> No, because if GPIOs *slow* can sleep and GPIO *cmd* can't sleep
>> it will call gpio_set_value_cansleep() for both.
> 
> If just one of them can sleep, can_sleep will be 0, and bad things
> will happen, right?

With current code if just one of them can sleep, can_sleep will be not zero (!= 0).
And bad things will happen.

> 
> 
>>>> @@ -205,7 +205,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
>>>> 	led_dat->cdev.groups = ns2_led_groups;
>>>> 	led_dat->cmd = template->cmd;
>>>> 	led_dat->slow = template->slow;
>>>> -	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
>>>> +	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) &&
>>>> 				gpio_cansleep(led_dat->slow);
>>>> 	if (led_dat->can_sleep)
>>>> 		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
> 
> Best regards,
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 2/2] leds: ns2: Fix wrong boolean expression

[Oleh Kravchenko]

Pavel,

> On Sep 3, 2019, at 9:50 PM, Pavel Machek <pavel@ucw.cz> wrote:
> 
> On Tue 2019-09-03 19:00:21, Oleh Kravchenko wrote:
>> Hello Pavel,
>> 
>> 03.09.19 17:12, Pavel Machek пише:
>>> On Tue 2019-09-03 15:50:20, Oleh Kravchenko wrote:
>>>> Error was detected by PVS-Studio:
>>>> V792 The '__gpio_cansleep' function located to the right of
>>>> the operator '|' will be called regardless of the value of
>>>> the left operand. Perhaps, it is better to use '||'.
>>> 1st: original code is not wrong
>> 
>> '|' is bitwise operation, if it really means *OR* then should be '||' -
>> led_dat->can_sleep is bool.
> 
> I see that || would be more natural. But | also works.
> 
>>> 2nd: you are introducing a bug
>> 
>> No, because if GPIOs *slow* can sleep and GPIO *cmd* can't sleep
>> it will call gpio_set_value_cansleep() for both.
> 
> If just one of them can sleep, can_sleep will be 0, and bad things
> will happen, right?

With current code if just one of them can sleep, can_sleep will be not zero (!= 0).
And bad things will happen.

> 
> 
>>>> @@ -205,7 +205,7 @@ create_ns2_led(struct platform_device *pdev, struct ns2_led_data *led_dat,
>>>> 	led_dat->cdev.groups = ns2_led_groups;
>>>> 	led_dat->cmd = template->cmd;
>>>> 	led_dat->slow = template->slow;
>>>> -	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) |
>>>> +	led_dat->can_sleep = gpio_cansleep(led_dat->cmd) &&
>>>> 				gpio_cansleep(led_dat->slow);
>>>> 	if (led_dat->can_sleep)
>>>> 		led_dat->cdev.brightness_set_blocking = ns2_led_set_blocking;
> 
> Best regards,
> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH 1/2] leds: mlxreg: Fix possible buffer overflow

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Tue 2019-09-03 15:50:19, Oleh Kravchenko wrote:
> Error was detected by PVS-Studio:
> V512 A call of the 'sprintf' function will lead to overflow of
> the buffer 'led_data->led_cdev_name'.

Are you sure this is correct fix? Will the name be always properly
null terminated?
									Pavel

> +++ b/drivers/leds/leds-mlxreg.c
> @@ -228,8 +228,8 @@ static int mlxreg_led_config(struct mlxreg_led_priv_data *priv)
>  			brightness = LED_OFF;
>  			led_data->base_color = MLXREG_LED_GREEN_SOLID;
>  		}
> -		sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
> -			data->label);
> +		snprintf(led_data->led_cdev_name, sizeof(led_data->led_cdev_name),
> +			 "mlxreg:%s", data->label);
>  		led_cdev->name = led_data->led_cdev_name;
>  		led_cdev->brightness = brightness;
>  		led_cdev->max_brightness = LED_ON;

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] leds: mlxreg: Fix possible buffer overflow

[Oleh Kravchenko]

Hi Pavel,

3 вер. 2019 р. о 17:12 Pavel Machek <pavel@ucw.cz> пише:

>> On Tue 2019-09-03 15:50:19, Oleh Kravchenko wrote:
>> Error was detected by PVS-Studio:
>> V512 A call of the 'sprintf' function will lead to overflow of
>> the buffer 'led_data->led_cdev_name'.
> 
> Are you sure this is correct fix? Will the name be always properly
> null terminated?

snprintf() always terminate string by NULL

>                                    Pavel
> 
>> +++ b/drivers/leds/leds-mlxreg.c
>> @@ -228,8 +228,8 @@ static int mlxreg_led_config(struct mlxreg_led_priv_data *priv)
>>            brightness = LED_OFF;
>>            led_data->base_color = MLXREG_LED_GREEN_SOLID;
>>        }
>> -        sprintf(led_data->led_cdev_name, "%s:%s", "mlxreg",
>> -            data->label);
>> +        snprintf(led_data->led_cdev_name, sizeof(led_data->led_cdev_name),
>> +             "mlxreg:%s", data->label);
>>        led_cdev->name = led_data->led_cdev_name;
>>        led_cdev->brightness = brightness;
>>        led_cdev->max_brightness = LED_ON;
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

--
Best regards,
Oleh Kravchenko

Re: [PATCH 1/2] leds: mlxreg: Fix possible buffer overflow

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Tue 2019-09-03 21:11:30, Oleh Kravchenko wrote:
> Hi Pavel,
> 
> 3 вер. 2019 р. о 17:12 Pavel Machek <pavel@ucw.cz> пише:
> 
> >> On Tue 2019-09-03 15:50:19, Oleh Kravchenko wrote:
> >> Error was detected by PVS-Studio:
> >> V512 A call of the 'sprintf' function will lead to overflow of
> >> the buffer 'led_data->led_cdev_name'.
> > 
> > Are you sure this is correct fix? Will the name be always properly
> > null terminated?
> 
> snprintf() always terminate string by NULL

You are right, sorry for the noise.
									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH 1/2] leds: mlxreg: Fix possible buffer overflow

[Oleh Kravchenko]

Hello Pavel,

> 7 вер. 2019 р. о 10:36 пп Pavel Machek <pavel@ucw.cz> написав(ла):
> 
> On Tue 2019-09-03 21:11:30, Oleh Kravchenko wrote:
>> Hi Pavel,
>> 
>> 3 вер. 2019 р. о 17:12 Pavel Machek <pavel@ucw.cz> пише:
>> 
>>>> On Tue 2019-09-03 15:50:19, Oleh Kravchenko wrote:
>>>> Error was detected by PVS-Studio:
>>>> V512 A call of the 'sprintf' function will lead to overflow of
>>>> the buffer 'led_data->led_cdev_name'.
>>> 
>>> Are you sure this is correct fix? Will the name be always properly
>>> null terminated?
>> 
>> snprintf() always terminate string by NULL
> 
> You are right, sorry for the noise.

No problem.
But what about patch? It’s ok for you?
I don’t see any Acked-by or Reviewed-by :-)

> 									Pavel
> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

-- 
Best regards,
Oleh Kravchenko