Discussion:
[PATCH] mtd: cast to u64 to avoid unexpected error
Huijin Park
2018-08-23 07:28:02 UTC
Permalink
From: "huijin.park" <***@samsung.com>

the params->size is defined as "u64"
and, "info->sector_size" and "info->n_sectors" is defined as unsgined and u16
thus, u64 data might have strange data(loss data) if data is overflow.
this patch cast it to u64.

Signed-off-by: huijin.park <***@samsung.com>
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d9c368c..527f281 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2459,7 +2459,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
memset(params, 0, sizeof(*params));

/* Set SPI NOR sizes. */
- params->size = info->sector_size * info->n_sectors;
+ params->size = (u64)info->sector_size * (u64)info->n_sectors;
params->page_size = info->page_size;

/* (Fast) Read settings. */
--
1.7.9.5
Boris Brezillon
2018-10-31 13:55:05 UTC
Permalink
Hi Huijin,

Subject prefix should be "mtd: spi-nor: ...", and please replace
"unexpected error" by "unsigned int overflows".

On Thu, 23 Aug 2018 03:28:02 -0400
Post by Huijin Park
the params->size is defined as "u64"
and, "info->sector_size" and "info->n_sectors" is defined as unsgined and u16
^ are ^ unsigned
Post by Huijin Park
thus, u64 data might have strange data(loss data) if data is overflow.
^ if the result
overflows an unsigned int.
Post by Huijin Park
this patch cast it to u64.
^casts info->sector_size to an u64.
Post by Huijin Park
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d9c368c..527f281 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2459,7 +2459,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
memset(params, 0, sizeof(*params));
/* Set SPI NOR sizes. */
- params->size = info->sector_size * info->n_sectors;
+ params->size = (u64)info->sector_size * (u64)info->n_sectors;
^ this cast is not needed.

BTW, I doubt we'll ever have to deal with NORs that are more than 4GB,
but making static code analysis tools happy and enforcing code
correctness is important too.
Post by Huijin Park
params->page_size = info->page_size;
/* (Fast) Read settings. */
Regards,

Boris
Huijin Park
2018-11-12 16:12:48 UTC
Permalink
Hi Boris

Thanks for review.
I will send patch again.

On Wed, Oct 31, 2018 at 10:55 PM Boris Brezillon
Post by Boris Brezillon
Hi Huijin,
Subject prefix should be "mtd: spi-nor: ...", and please replace
"unexpected error" by "unsigned int overflows".
On Thu, 23 Aug 2018 03:28:02 -0400
Post by Huijin Park
the params->size is defined as "u64"
and, "info->sector_size" and "info->n_sectors" is defined as unsgined and u16
^ are ^ unsigned
Post by Huijin Park
thus, u64 data might have strange data(loss data) if data is overflow.
^ if the result
overflows an unsigned int.
Post by Huijin Park
this patch cast it to u64.
^casts info->sector_size to an u64.
Post by Huijin Park
---
drivers/mtd/spi-nor/spi-nor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d9c368c..527f281 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2459,7 +2459,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
memset(params, 0, sizeof(*params));
/* Set SPI NOR sizes. */
- params->size = info->sector_size * info->n_sectors;
+ params->size = (u64)info->sector_size * (u64)info->n_sectors;
^ this cast is not needed.
BTW, I doubt we'll ever have to deal with NORs that are more than 4GB,
but making static code analysis tools happy and enforcing code
correctness is important too.
I agree that enforcing code correctness is important as your said too.
Post by Boris Brezillon
Post by Huijin Park
params->page_size = info->page_size;
/* (Fast) Read settings. */
Regards,
Boris
Thanks & best regards,

Huijin

Loading...