Skip to contentSkip to conversation

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
#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;
#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

author
#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

author
#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

reviewer
#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

maintainer Acked-by on v1 1/1
#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]>
Applied
#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