* [PATCH 1/1] cmd: avoid overflow in mtd_is_aligned_with_min_io_size
@ 2023-07-30 13:04 Heinrich Schuchardt
2023-07-31 7:00 ` Michael Nazzareno Trimarchi
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2023-07-30 13:04 UTC (permalink / raw)
To: Tom Rini
Cc: Dario Binacchi, Michael Trimarchi, Mikhail Kshevetskiy, u-boot,
Heinrich Schuchardt
Multiplication of u32 factors has an u32 result even if an overflow occurs.
An overflow may occur in
u64 data_off = page * mtd->writesize;
The size of the flash device may exceed 4 GiB.
Play it safe and use u64 consistently.
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
cmd/mtd.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/cmd/mtd.c b/cmd/mtd.c
index eb6e2d6892..fb6e2bb047 100644
--- a/cmd/mtd.c
+++ b/cmd/mtd.c
@@ -33,11 +33,9 @@ static struct mtd_info *get_mtd_by_name(const char *name)
return mtd;
}
-static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
+static u64 mtd_len_to_pages(struct mtd_info *mtd, u64 len)
{
- do_div(len, mtd->writesize);
-
- return len;
+ return lldiv(len, mtd->writesize);
}
static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd, u64 size)
@@ -72,8 +70,7 @@ static void mtd_dump_device_buf(struct mtd_info *mtd, u64 start_off,
{
bool has_pages = mtd->type == MTD_NANDFLASH ||
mtd->type == MTD_MLCNANDFLASH;
- int npages = mtd_len_to_pages(mtd, len);
- uint page;
+ u64 page, npages = mtd_len_to_pages(mtd, len);
if (has_pages) {
for (page = 0; page < npages; page++) {
@@ -251,12 +248,12 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
char *const argv[])
{
bool dump, read, raw, woob, write_empty_pages, has_pages = false;
- u64 start_off, off, len, remaining, default_len;
+ u64 start_off, off, len, oob_len, remaining, default_len;
struct mtd_oob_ops io_op = {};
- uint user_addr = 0, npages;
+ uint user_addr = 0;
+ u64 npages;
const char *cmd = argv[0];
struct mtd_info *mtd;
- u32 oob_len;
u8 *buf;
int ret;
@@ -322,7 +319,7 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
}
if (has_pages)
- printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
+ printf("%s %lld byte(s) (%lld page(s)) at offset 0x%08llx%s%s%s\n",
read ? "Reading" : "Writing", len, npages, start_off,
raw ? " [raw]" : "", woob ? " [oob]" : "",
!read && write_empty_pages ? " [dontskipff]" : "");
--
2.40.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] cmd: avoid overflow in mtd_is_aligned_with_min_io_size
2023-07-30 13:04 [PATCH 1/1] cmd: avoid overflow in mtd_is_aligned_with_min_io_size Heinrich Schuchardt
@ 2023-07-31 7:00 ` Michael Nazzareno Trimarchi
2023-07-31 8:10 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-07-31 7:00 UTC (permalink / raw)
To: Heinrich Schuchardt; +Cc: Tom Rini, Dario Binacchi, Mikhail Kshevetskiy, u-boot
Hi
On Sun, Jul 30, 2023 at 3:03 PM Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Multiplication of u32 factors has an u32 result even if an overflow occurs.
> An overflow may occur in
>
> u64 data_off = page * mtd->writesize;
>
> The size of the flash device may exceed 4 GiB.
>
Instead of force variables to u64 just cast where expansion is needed before
multiple. Why we should thinking to have u64 number of nand pages?
Michael
> Play it safe and use u64 consistently.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> cmd/mtd.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/cmd/mtd.c b/cmd/mtd.c
> index eb6e2d6892..fb6e2bb047 100644
> --- a/cmd/mtd.c
> +++ b/cmd/mtd.c
> @@ -33,11 +33,9 @@ static struct mtd_info *get_mtd_by_name(const char *name)
> return mtd;
> }
>
> -static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
> +static u64 mtd_len_to_pages(struct mtd_info *mtd, u64 len)
> {
> - do_div(len, mtd->writesize);
> -
> - return len;
> + return lldiv(len, mtd->writesize);
> }
>
> static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd, u64 size)
> @@ -72,8 +70,7 @@ static void mtd_dump_device_buf(struct mtd_info *mtd, u64 start_off,
> {
> bool has_pages = mtd->type == MTD_NANDFLASH ||
> mtd->type == MTD_MLCNANDFLASH;
> - int npages = mtd_len_to_pages(mtd, len);
> - uint page;
> + u64 page, npages = mtd_len_to_pages(mtd, len);
>
> if (has_pages) {
> for (page = 0; page < npages; page++) {
> @@ -251,12 +248,12 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
> char *const argv[])
> {
> bool dump, read, raw, woob, write_empty_pages, has_pages = false;
> - u64 start_off, off, len, remaining, default_len;
> + u64 start_off, off, len, oob_len, remaining, default_len;
> struct mtd_oob_ops io_op = {};
> - uint user_addr = 0, npages;
> + uint user_addr = 0;
> + u64 npages;
> const char *cmd = argv[0];
> struct mtd_info *mtd;
> - u32 oob_len;
> u8 *buf;
> int ret;
>
> @@ -322,7 +319,7 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
> }
>
> if (has_pages)
> - printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
> + printf("%s %lld byte(s) (%lld page(s)) at offset 0x%08llx%s%s%s\n",
> read ? "Reading" : "Writing", len, npages, start_off,
> raw ? " [raw]" : "", woob ? " [oob]" : "",
> !read && write_empty_pages ? " [dontskipff]" : "");
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] cmd: avoid overflow in mtd_is_aligned_with_min_io_size
2023-07-31 7:00 ` Michael Nazzareno Trimarchi
@ 2023-07-31 8:10 ` Heinrich Schuchardt
2023-07-31 18:13 ` Michael Nazzareno Trimarchi
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2023-07-31 8:10 UTC (permalink / raw)
To: Michael Nazzareno Trimarchi
Cc: Tom Rini, Dario Binacchi, Mikhail Kshevetskiy, u-boot
On 7/31/23 09:00, Michael Nazzareno Trimarchi wrote:
> Hi
>
> On Sun, Jul 30, 2023 at 3:03 PM Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Multiplication of u32 factors has an u32 result even if an overflow occurs.
>> An overflow may occur in
>>
>> u64 data_off = page * mtd->writesize;
>>
>> The size of the flash device may exceed 4 GiB.
>>
>
> Instead of force variables to u64 just cast where expansion is needed before
> multiple. Why we should thinking to have u64 number of nand pages?
Does anything stop MTD devices from reaching 2^32 pages?
The driver layer uses loff_t for 'from'. I can't see any limitation there.
Best regards
Heinrich
>
> Michael
>
>> Play it safe and use u64 consistently.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> cmd/mtd.c | 17 +++++++----------
>> 1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/cmd/mtd.c b/cmd/mtd.c
>> index eb6e2d6892..fb6e2bb047 100644
>> --- a/cmd/mtd.c
>> +++ b/cmd/mtd.c
>> @@ -33,11 +33,9 @@ static struct mtd_info *get_mtd_by_name(const char *name)
>> return mtd;
>> }
>>
>> -static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
>> +static u64 mtd_len_to_pages(struct mtd_info *mtd, u64 len)
>> {
>> - do_div(len, mtd->writesize);
>> -
>> - return len;
>> + return lldiv(len, mtd->writesize);
>> }
>>
>> static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd, u64 size)
>> @@ -72,8 +70,7 @@ static void mtd_dump_device_buf(struct mtd_info *mtd, u64 start_off,
>> {
>> bool has_pages = mtd->type == MTD_NANDFLASH ||
>> mtd->type == MTD_MLCNANDFLASH;
>> - int npages = mtd_len_to_pages(mtd, len);
>> - uint page;
>> + u64 page, npages = mtd_len_to_pages(mtd, len);
>>
>> if (has_pages) {
>> for (page = 0; page < npages; page++) {
>> @@ -251,12 +248,12 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
>> char *const argv[])
>> {
>> bool dump, read, raw, woob, write_empty_pages, has_pages = false;
>> - u64 start_off, off, len, remaining, default_len;
>> + u64 start_off, off, len, oob_len, remaining, default_len;
>> struct mtd_oob_ops io_op = {};
>> - uint user_addr = 0, npages;
>> + uint user_addr = 0;
>> + u64 npages;
>> const char *cmd = argv[0];
>> struct mtd_info *mtd;
>> - u32 oob_len;
>> u8 *buf;
>> int ret;
>>
>> @@ -322,7 +319,7 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int flag, int argc,
>> }
>>
>> if (has_pages)
>> - printf("%s %lld byte(s) (%d page(s)) at offset 0x%08llx%s%s%s\n",
>> + printf("%s %lld byte(s) (%lld page(s)) at offset 0x%08llx%s%s%s\n",
>> read ? "Reading" : "Writing", len, npages, start_off,
>> raw ? " [raw]" : "", woob ? " [oob]" : "",
>> !read && write_empty_pages ? " [dontskipff]" : "");
>> --
>> 2.40.1
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/1] cmd: avoid overflow in mtd_is_aligned_with_min_io_size
2023-07-31 8:10 ` Heinrich Schuchardt
@ 2023-07-31 18:13 ` Michael Nazzareno Trimarchi
0 siblings, 0 replies; 4+ messages in thread
From: Michael Nazzareno Trimarchi @ 2023-07-31 18:13 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Tom Rini, Dario Binacchi, Mikhail Kshevetskiy, U-Boot-Denx
Hi
On Mon, Jul 31, 2023 at 10:10 AM Heinrich Schuchardt <
heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 7/31/23 09:00, Michael Nazzareno Trimarchi wrote:
> > Hi
> >
> > On Sun, Jul 30, 2023 at 3:03 PM Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Multiplication of u32 factors has an u32 result even if an overflow
occurs.
> >> An overflow may occur in
> >>
> >> u64 data_off = page * mtd->writesize;
> >>
> >> The size of the flash device may exceed 4 GiB.
> >>
> >
> > Instead of force variables to u64 just cast where expansion is needed
before
> > multiple. Why we should thinking to have u64 number of nand pages?
>
> Does anything stop MTD devices from reaching 2^32 pages?
>
Pages is integer right now in the code. What is the problem just to fix
what you are claim
Michael
> The driver layer uses loff_t for 'from'. I can't see any limitation there.
>
> Best regards
>
> Heinrich
>
> >
> > Michael
> >
> >> Play it safe and use u64 consistently.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >> cmd/mtd.c | 17 +++++++----------
> >> 1 file changed, 7 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/cmd/mtd.c b/cmd/mtd.c
> >> index eb6e2d6892..fb6e2bb047 100644
> >> --- a/cmd/mtd.c
> >> +++ b/cmd/mtd.c
> >> @@ -33,11 +33,9 @@ static struct mtd_info *get_mtd_by_name(const char
*name)
> >> return mtd;
> >> }
> >>
> >> -static uint mtd_len_to_pages(struct mtd_info *mtd, u64 len)
> >> +static u64 mtd_len_to_pages(struct mtd_info *mtd, u64 len)
> >> {
> >> - do_div(len, mtd->writesize);
> >> -
> >> - return len;
> >> + return lldiv(len, mtd->writesize);
> >> }
> >>
> >> static bool mtd_is_aligned_with_min_io_size(struct mtd_info *mtd,
u64 size)
> >> @@ -72,8 +70,7 @@ static void mtd_dump_device_buf(struct mtd_info
*mtd, u64 start_off,
> >> {
> >> bool has_pages = mtd->type == MTD_NANDFLASH ||
> >> mtd->type == MTD_MLCNANDFLASH;
> >> - int npages = mtd_len_to_pages(mtd, len);
> >> - uint page;
> >> + u64 page, npages = mtd_len_to_pages(mtd, len);
> >>
> >> if (has_pages) {
> >> for (page = 0; page < npages; page++) {
> >> @@ -251,12 +248,12 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int
flag, int argc,
> >> char *const argv[])
> >> {
> >> bool dump, read, raw, woob, write_empty_pages, has_pages =
false;
> >> - u64 start_off, off, len, remaining, default_len;
> >> + u64 start_off, off, len, oob_len, remaining, default_len;
> >> struct mtd_oob_ops io_op = {};
> >> - uint user_addr = 0, npages;
> >> + uint user_addr = 0;
> >> + u64 npages;
> >> const char *cmd = argv[0];
> >> struct mtd_info *mtd;
> >> - u32 oob_len;
> >> u8 *buf;
> >> int ret;
> >>
> >> @@ -322,7 +319,7 @@ static int do_mtd_io(struct cmd_tbl *cmdtp, int
flag, int argc,
> >> }
> >>
> >> if (has_pages)
> >> - printf("%s %lld byte(s) (%d page(s)) at offset
0x%08llx%s%s%s\n",
> >> + printf("%s %lld byte(s) (%lld page(s)) at offset
0x%08llx%s%s%s\n",
> >> read ? "Reading" : "Writing", len, npages,
start_off,
> >> raw ? " [raw]" : "", woob ? " [oob]" : "",
> >> !read && write_empty_pages ? " [dontskipff]" :
"");
> >> --
> >> 2.40.1
> >>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-07-31 18:13 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-30 13:04 [PATCH 1/1] cmd: avoid overflow in mtd_is_aligned_with_min_io_size Heinrich Schuchardt
2023-07-31 7:00 ` Michael Nazzareno Trimarchi
2023-07-31 8:10 ` Heinrich Schuchardt
2023-07-31 18:13 ` Michael Nazzareno Trimarchi
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.