powerpc: signedness bug in update_flash_db()

PATCH Applied Dan Carpenter 5 participants 7 messages Oct 1, 2018 – Oct 15, 2018

Subsystems (3)

PowerPC Kernel janitors PS3 platform support

lore

Share & tools

lore

Dan Carpenter

author

2018-10-01 16:44 UTC #0 lore raw

PATCH 1/1 powerpc: signedness bug in update_flash_db()

The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.

This doesn't really change runtime much but it fixes a static checker warning.

Signed-off-by Dan Carpenter <[email protected]>

arch/powerpc/platforms/ps3/os-area.c +1 -1

index cdbfc5cfd6f3..f5387ad82279 100644
@@ -664,7 +664,7 @@ static int update_flash_db(void)
 	db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
 
 	count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
-	if (count < sizeof(struct os_area_db)) {
+	if (count < 0 || count < sizeof(struct os_area_db)) {
 		pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
 			 count);
 		error = count < 0 ? count : -EIO;

christophe leroy

2018-10-01 18:22 UTC #1 lore raw

Le 01/10/2018 à 18:44, Dan Carpenter a écrit :

Quoted 14 lines

The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker warning.
Signed-off-by Dan Carpenter <[email protected]>
arch/powerpc/platforms/ps3/os-area.c +0 -0
index cdbfc5cfd6f3..f5387ad82279 100644

arch/powerpc/platforms/ps3/os-area.c static int update_flash_db(void) · line 667

Quoted 5 lines

arch/powerpc/platforms/ps3/os-area.c +1 -1

@@ -664,7 +664,7 @@ static int update_flash_db(void)
  	db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
 
  	count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
-	if (count < sizeof(struct os_area_db)) {
+	if (count < 0 || count < sizeof(struct os_area_db)) {

Why not simply add a cast ? :

if (count < (ssize_t)sizeof(struct os_area_db)) {

Christophe

arch/powerpc/platforms/ps3/os-area.c static int update_flash_db(void) · line 670

Quoted 4 lines

arch/powerpc/platforms/ps3/os-area.c +0 -0

  		pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
  			 count);
  		error = count < 0 ? count : -EIO;

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Dan Carpenter

author

2018-10-01 19:02 UTC #2 lore raw

Quoted 24 lines · christophe leroy

Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
Quoted 14 lines
The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker warning.
Signed-off-by Dan Carpenter <[email protected]>
arch/powerpc/platforms/ps3/os-area.c +0 -0
index cdbfc5cfd6f3..f5387ad82279 100644
arch/powerpc/platforms/ps3/os-area.c static int update_flash_db(void) · line 666
Quoted 4 lines
arch/powerpc/platforms/ps3/os-area.c +1 -1
@@ -664,7 +664,7 @@ static int update_flash_db(void)
  	db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
  	count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
-	if (count < sizeof(struct os_area_db)) {
+	if (count < 0 || count < sizeof(struct os_area_db)) {
Why not simply add a cast ? :

Quoted 2 lines

if (count < (ssize_t)sizeof(struct os_area_db)) {

There are so many ways to solve these and no accounting for taste. Do you need me to resend or can you redo it yourself?

regards,
dan carpenter

Dan Carpenter

author

2018-10-01 19:06 UTC #3 lore raw

Quoted 25 lines · Dan Carpenter

Quoted 24 lines · christophe leroy
Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
Quoted 14 lines
The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker warning.
Signed-off-by Dan Carpenter <[email protected]>
arch/powerpc/platforms/ps3/os-area.c +0 -0
index cdbfc5cfd6f3..f5387ad82279 100644
arch/powerpc/platforms/ps3/os-area.c static int update_flash_db(void) · line 666
Quoted 4 lines
arch/powerpc/platforms/ps3/os-area.c +1 -1
@@ -664,7 +664,7 @@ static int update_flash_db(void)
  	db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
  	count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
-	if (count < sizeof(struct os_area_db)) {
+	if (count < 0 || count < sizeof(struct os_area_db)) {
Why not simply add a cast ? :

Quoted 6 lines

Quoted 2 lines
if (count < (ssize_t)sizeof(struct os_area_db)) {
There are so many ways to solve these and no accounting for taste. Do you need me to resend or can you redo it yourself?

Btw, I just went on vacation, and I'm not going to be back until next week.

regards,
dan carpenter

Geoff Levand

maintainer Acked-by on v1 1/1

2018-10-01 22:45 UTC #5 lore raw

In reply to Dan Carpenter

Quoted 15 lines · Dan Carpenter

The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker warning.
Signed-off-by Dan Carpenter <[email protected]>
arch/powerpc/platforms/ps3/os-area.c +0 -0
index cdbfc5cfd6f3..f5387ad82279 100644

arch/powerpc/platforms/ps3/os-area.c static int update_flash_db(void) · line 670

Quoted 8 lines

arch/powerpc/platforms/ps3/os-area.c +1 -1

@@ -664,7 +664,7 @@ static int update_flash_db(void)
 	db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
 
 	count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
-	if (count < sizeof(struct os_area_db)) {
+	if (count < 0 || count < sizeof(struct os_area_db)) {
 		pr_debug("%s: os_area_flash_write failed %zd\n", __func__,
 			 count);
 		error = count < 0 ? count : -EIO;

Seems OK.

Acked-by Geoff Levand <[email protected]>

Michael Ellerman

reviewer

2018-10-09 11:54 UTC #4 lore raw

In reply to christophe leroy

Quoted 17 lines · christophe leroy

Le 01/10/2018 à 18:44, Dan Carpenter a écrit :
Quoted 16 lines
The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker warning.
Signed-off-by Dan Carpenter <[email protected]>
arch/powerpc/platforms/ps3/os-area.c +0 -0
index cdbfc5cfd6f3..f5387ad82279 100644
@@ -664,7 +664,7 @@ static int update_flash_db(void)
  	db_set_64(db, &os_area_db_id_rtc_diff, saved_params.rtc_diff);
 

Quoted 7 lines

arch/powerpc/platforms/ps3/os-area.c static int update_flash_db(void) · line 667
Quoted 3 lines
arch/powerpc/platforms/ps3/os-area.c +1 -1
  	count = os_area_flash_write(db, sizeof(struct os_area_db), pos);
-	if (count < sizeof(struct os_area_db)) {
+	if (count < 0 || count < sizeof(struct os_area_db)) {
Why not simply add a cast ? :
if (count < (ssize_t)sizeof(struct os_area_db)) {

The explicit check against 0 is much clearer IMO.

The original author and all reviewers since obviously didn't realise that count was being implicitly cast, so fixing that with another cast seems likely to just confuse people even more :)

cheers

Michael Ellerman

Applied

2018-10-15 04:01 UTC #6 lore raw

In reply to Dan Carpenter

Quoted 9 lines · Dan Carpenter

The "count < sizeof(struct os_area_db)" comparison is type promoted to size_t so negative values of "count" are treated as very high values and we accidentally return success instead of a negative error code.
This doesn't really change runtime much but it fixes a static checker warning.
Signed-off-by Dan Carpenter <[email protected]>
Acked-by Geoff Levand <[email protected]>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/014704e6f54189a203cc14c7c0bb41

cheers