[PATCH v5] env: mmc: Correct partition comparison in mmc_offset_try_partition
The function mmc_offset_try_partition searches the MMC partition for
locating environment data, by comparing the partition names with config
"u-boot,mmc-env-parition". However, it only compares the first word-size
bytes (size of 'const char *'), which may make the function to find
unintended partition.
Correct the function not to partially compare the partition name with
config "u-boot,mmc-env-partition".
Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
Signed-off-by: Hoyeonjiki Kim
Reviewed-by: Wolfgang Denk
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index 4e67180b23..ee376c3e0c 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str,
int copy, s64 *val)
if (ret < 0)
return ret;
- if (!strncmp((const char *)info.name, str, sizeof(str)))
+ if (!strncmp((const char *)info.name, str, sizeof(info.name)))
break;
}
--
2.25.1
Re: [PATCH v3] env: mmc: Correct partition comparison in mmc_offset_try_partition
Dear Wolfgang Denk,
On Tue, Nov 17, 2020 at 4:17 AM Wolfgang Denk wrote:
>
> Dear Hoyeonjiki Kim,
>
> In message <20201115172544.548-1-jigi@gmail.com> you wrote:
> > The function mmc_offset_try_partition searches the MMC partition for
> > locating environment data, by comparing the partition names with config
> > "u-boot,mmc-env-parition". However, it only compares the first word-size
> > bytes (size of 'const char *'), which may make the function to find
> > unintended partition.
> >
> > Correct the function not to partially compare the partition name with
> > config "u-boot,mmc-env-partition".
> >
> > Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
> > Signed-off-by: Hoyeonjiki Kim
> > Reviewed-by: Jaehoon Chung
> > Reviewed-by: Jorge Ramire-Ortiz
> > Reviewed-by: Wolfgang Denk
> > ---
>
> Reviewed-by: Wolfgang Denk
>
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
> God made the integers; all else is the work of Man. - Kronecker
Thanks for all the reviews for this patch.
I'll bring the patch with it.
Best Regards,
Hoyeonjiki Kim
[PATCH v4] env: mmc: Correct partition comparison in mmc_offset_try_partition
The function mmc_offset_try_partition searches the MMC partition for
locating environment data, by comparing the partition names with config
"u-boot,mmc-env-parition". However, it only compares the first word-size
bytes (size of 'const char *'), which may make the function to find
unintended partition.
Correct the function not to partially compare the partition name with
config "u-boot,mmc-env-partition".
Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
Signed-off-by: Hoyeonjiki Kim
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index 4e67180b23..ee376c3e0c 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str,
int copy, s64 *val)
if (ret < 0)
return ret;
- if (!strncmp((const char *)info.name, str, sizeof(str)))
+ if (!strncmp((const char *)info.name, str, sizeof(info.name)))
break;
}
--
2.25.1
Re: [PATCH v3] env: mmc: Correct partition comparison in mmc_offset_try_partition
Jaehoon Chung,
On Mon, Nov 16, 2020 at 7:35 AM Jaehoon Chung wrote:
>
> Dear Hoyeonjiki,
>
> On 11/16/20 2:25 AM, Hoyeonjiki Kim wrote:
> > The function mmc_offset_try_partition searches the MMC partition for
> > locating environment data, by comparing the partition names with config
> > "u-boot,mmc-env-parition". However, it only compares the first word-size
> > bytes (size of 'const char *'), which may make the function to find
> > unintended partition.
> >
> > Correct the function not to partially compare the partition name with
> > config "u-boot,mmc-env-partition".
> >
> > Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
> > Signed-off-by: Hoyeonjiki Kim
> > Reviewed-by: Jaehoon Chung
> > Reviewed-by: Jorge Ramire-Ortiz
> > Reviewed-by: Wolfgang Denk
>
> Nobody replied with reviewed-by tag.
> Don't add reviewed-by tag yourself.
>
> Best Regards,
> Jaehoon Chung
Seems I misunderstood how it works...
Thanks for letting me know.
I will resend my patch with those reviewed-by tags.
Best Regards,
Hoyeonjiki Kim
>
> > ---
> > env/mmc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/env/mmc.c b/env/mmc.c
> > index 4e67180b23..ee376c3e0c 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char
> > *str, int copy, s64 *val)
> > if (ret < 0)
> > return ret;
> >
> > - if (!strncmp((const char *)info.name, str, sizeof(str)))
> > + if (!strncmp((const char *)info.name, str, sizeof(info.name)))
> > break;> }
> >
> >
>
[PATCH v3] env: mmc: Correct partition comparison in mmc_offset_try_partition
The function mmc_offset_try_partition searches the MMC partition for
locating environment data, by comparing the partition names with config
"u-boot,mmc-env-parition". However, it only compares the first word-size
bytes (size of 'const char *'), which may make the function to find
unintended partition.
Correct the function not to partially compare the partition name with
config "u-boot,mmc-env-partition".
Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
Signed-off-by: Hoyeonjiki Kim
Reviewed-by: Jaehoon Chung
Reviewed-by: Jorge Ramire-Ortiz
Reviewed-by: Wolfgang Denk
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index 4e67180b23..ee376c3e0c 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str,
int copy, s64 *val)
if (ret < 0)
return ret;
- if (!strncmp((const char *)info.name, str, sizeof(str)))
+ if (!strncmp((const char *)info.name, str, sizeof(info.name)))
break;
}
--
2.25.1
Re: [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition
Dear Wolfgang Denk, On Mon, Nov 16, 2020 at 1:36 AM Wolfgang Denk wrote: > > Dear Hoyeonjiki Kim, > > In message > you > wrote: > > > > As you referred, `strcmp` suffers with non-null terminated string(s). > > I'd also checked if using `strcmp` can cause some issues and > > seems it's **guaranteed** that there is no such issue in this context. > > You ar4e probably right, but the problem with this approach is that > what today is a verified context, may tomoroow change - a new use > case may be added, which is not aware of this potential problem, and > which thus triggers a (foreseeable and avoidable bug). Absolutely. I got your point and agree with that. > > > But if we need to specify that the context will not suffer anyway, there > > is an option to use `strncmp` with `PART_NAME_LEN` as max count param. > > > > `PART_NAME_LEN` is the size of `info.name` which is a character buffer. > > If we know we size (and apparewntly we do), we should use this with > strncmp(). Just in case... Because in this context, we can use 'sizeof(info.name)' as max count, I think I can bring v3 with strncmp(). Thanks for the feedback. Best Regards, Hoyeonjiki Kim > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de > It's not what you do, it's how you do what you do! - Jordan D. Ulmer
Re: [PATCH] env: mmc: Correct partition comparison in mmc_offset_try_partition
On Fri, Nov 13, 2020 at 5:06 AM Wolfgang Denk wrote:
>
> Dear Hoyeonjiki Kim,
>
> In message <20201112123026.458-1-jigi@gmail.com> you wrote:
> > The function mmc_offset_try_partition searches MMC partition to save the
> > environment data by name. However, it only compares the first word-size
> > bytes (size of 'const char *'), which may make the function to find
> > unintended partition.
> >
> > Correct the function not to partially compare the partition name with
> > config "u-boot,mmc-env-partition".
> >
> > Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
> > Signed-off-by: Hoyeonjiki Kim
> > Reviewed-by: Jaehoon Chung
> > ---
> > env/mmc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/env/mmc.c b/env/mmc.c
> > index 4e67180b23..505f7aa2b8 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char
> > *str, int copy, s64 *val)
> > if (ret < 0)
> > return ret;
> >
> > - if (!strncmp((const char *)info.name, str, sizeof(str)))
> > + if (!strcmp((const char *)info.name, str))
>
> This looks dangerous to me. If the used length sizeof(str) is not
> correct, that should be fixed, but switching to strcmp() means we
> may run into problems if the buffer is not NUL terminated. Do we
> have a guarantee here thai it always ist?? The fact that strncmp()
> has been used before suggests otyherwise.
>
> Please double-check.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
> Computers are not intelligent. They only think they are.
Dear Wolfgang Denk,
I left my reply on the thread for patch v2.
Please refer to that.
We can keep discussing it on that thread from now on.
Again, sorry for the confusion.
Best Regards,
Hoyeonjiki Kim
Re: [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition
On Fri, Nov 13, 2020 at 5:07 AM Wolfgang Denk wrote:
>
> Dear Hoyeonjiki Kim,
>
> In message <20201112131237.1239-1-jigi@gmail.com> you wrote:
> > The function mmc_offset_try_partition searches MMC partition to save the
> > environment data by name. However, it only compares the first word-size
> > bytes (size of 'const char *'), which may make the function to find
> > unintended partition.
> >
> > Correct the function not to partially compare the partition name with
> > config "u-boot,mmc-env-partition".
> >
> > Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
> > Signed-off-by: Hoyeonjiki Kim
> > Reviewed-by: Jaehoon Chung
> > ---
> > env/mmc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/env/mmc.c b/env/mmc.c
> > index 4e67180b23..505f7aa2b8 100644
> > --- a/env/mmc.c
> > +++ b/env/mmc.c
> > @@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char
> > *str, int copy, s64 *val)
> > if (ret < 0)
> > return ret;
> >
> > - if (!strncmp((const char *)info.name, str, sizeof(str)))
> > + if (!strcmp((const char *)info.name, str))
>
> Resend my comment, too. This looks dangerous, please double check!!
Dear Wolfgang Denk,
Thanks for your feedback.
As you referred, `strcmp` suffers with non-null terminated string(s).
I'd also checked if using `strcmp` can cause some issues and
seems it's **guaranteed** that there is no such issue in this context.
Here's why:
- the first input string, `info.name` comes from fn `part_get_info`
which gets the partition info from one of the partition driver.
Each driver will return the partition info with null terminated
partition name (actually it must, or every part in U-Boot referring
`info.name` will have potential issues), so the first input string
is safe to use in `strcmp`.
- The second one, `str` comes from fn `fdtdec_get_config_string` which
gets the 'u-boot,mmc-env-offset' property value from FDT.
When you keep tracking that function, you will meet `fdt_get_string`
which returns error (-FDT_ERR_TRUNCATED) if the property value is
non-null terminated. So the second input string also is safe to use.
fdtdec_get_config_string
--> fdt_getprop
--> fdt_getprop_namelen
--> fdt_get_property_namelen_
--> fdt_string_eq_
--> fdt_get_stringa
For this reason, I think we can use `strcmp` in this context.
But if we need to specify that the context will not suffer anyway, there
is an option to use `strncmp` with `PART_NAME_LEN` as max count param.
`PART_NAME_LEN` is the size of `info.name` which is a character buffer.
Please let me know your opinion.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
> In Nature there are neither rewards nor punishments, there are conse-
> quences.-- R.G. Ingersoll
[PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition
The function mmc_offset_try_partition searches MMC partition to save the
environment data by name. However, it only compares the first word-size
bytes (size of 'const char *'), which may make the function to find
unintended partition.
Correct the function not to partially compare the partition name with
config "u-boot,mmc-env-partition".
Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
Signed-off-by: Hoyeonjiki Kim
Reviewed-by: Jaehoon Chung
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index 4e67180b23..505f7aa2b8 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str,
int copy, s64 *val)
if (ret < 0)
return ret;
- if (!strncmp((const char *)info.name, str, sizeof(str)))
+ if (!strcmp((const char *)info.name, str))
break;
}
--
2.25.1
Re: [PATCH] env: mmc: Correct partition comparison in mmc_offset_try_partition
Mis-submitted the patch without versioning... :(
I'll re-submit my patch again.
Sorry for interrupting you.
Best Regards,
Hoyeonjiki Kim
On Thu, Nov 12, 2020 at 9:30 PM Hoyeonjiki Kim wrote:
>
> The function mmc_offset_try_partition searches MMC partition to save the
> environment data by name. However, it only compares the first word-size
> bytes (size of 'const char *'), which may make the function to find
> unintended partition.
>
> Correct the function not to partially compare the partition name with
> config "u-boot,mmc-env-partition".
>
> Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
> Signed-off-by: Hoyeonjiki Kim
> Reviewed-by: Jaehoon Chung
> ---
> env/mmc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/env/mmc.c b/env/mmc.c
> index 4e67180b23..505f7aa2b8 100644
> --- a/env/mmc.c
> +++ b/env/mmc.c
> @@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str,
> int copy, s64 *val)
> if (ret < 0)
> return ret;
>
> - if (!strncmp((const char *)info.name, str, sizeof(str)))
> + if (!strcmp((const char *)info.name, str))
> break;
> }
>
> --
> 2.25.1
>
[PATCH] env: mmc: Correct partition comparison in mmc_offset_try_partition
The function mmc_offset_try_partition searches MMC partition to save the
environment data by name. However, it only compares the first word-size
bytes (size of 'const char *'), which may make the function to find
unintended partition.
Correct the function not to partially compare the partition name with
config "u-boot,mmc-env-partition".
Fixes: c9e87ba66540 ("env: Save environment at the end of an MMC partition")
Signed-off-by: Hoyeonjiki Kim
Reviewed-by: Jaehoon Chung
---
env/mmc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/env/mmc.c b/env/mmc.c
index 4e67180b23..505f7aa2b8 100644
--- a/env/mmc.c
+++ b/env/mmc.c
@@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str,
int copy, s64 *val)
if (ret < 0)
return ret;
- if (!strncmp((const char *)info.name, str, sizeof(str)))
+ if (!strcmp((const char *)info.name, str))
break;
}
--
2.25.1
Re: [PATCH] env: mmc: Correct partition comparison in mmc_offset_try_partition
On Thu, Nov 12, 2020 at 8:34 AM Jaehoon Chung wrote: > > Hi, > > On 11/11/20 7:25 PM, 김호연지기 wrote: > > On Wed, Nov 11, 2020 at 7:07 PM Jorge Ramirez-Ortiz, Gmail > > wrote: > >> > >> On 11/11/20, Jaehoon Chung wrote: > >>> On 11/10/20 11:28 PM, Hoyeonjiki Kim wrote: > >>>> The function mmc_offset_try_partition searches MMC partition to save the > >>>> environment data by name. However, it only compares the first word-size > >>>> bytes (size of 'const char *'), which may make the function to find > >>>> unintended partition. > >>>> > >>>> Correct the function not to partially compare the partition name with > >>>> config "u-boot,,mmc-env-partition". > >>>> > >>>> Signed-off-by: Hoyeonjiki Kim > >>>> --- > >>>> env/mmc.c | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/env/mmc.c b/env/mmc.c > >>>> index 4e67180b23..505f7aa2b8 100644 > >>>> --- a/env/mmc.c > >>>> +++ b/env/mmc.c > >>>> @@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char > >>>> *str, int copy, s64 *val) > >>>> if (ret < 0) > >>>> return ret; > >>>> > >>>> - if (!strncmp((const char *)info.name, str, sizeof(str))) > >>>> + if (!strcmp((const char *)info.name, str)) > >>> > >>> Using "strlen(str)" is better than changing to strcmp. > >>> > >>> strncmp(..., ..., strlen(str)) > >> > >> absolutely. > >> maybe also modify the commit like to indicate a fix to the current bug? > > > > Thanks for the feedback for both. > > > > However, when we do so, isn't there still the possibility for the > > function searching incorrect partition if, > > 1) "str" is shorter than "info.name", and > > 2) the first "strlen(str)" letters of "info.name" is same with those of > > "str"? > > Make sense. You're right. :) > > There are two approach. One is that choose one of those length what is longer. > Other is your approach. I don't have any objection to fix whatever. > > Just resend your patch with Jorge's comment, plz. > > > Best Regards, > Jaehoon Chung Sure, thanks for your opinion. Will be back with v2. Best Regards, Hoyeonjiki Kim > > > > > This commit is for fixing the current bug, but also I wanna make sure > > that partition name matches fully. > > > > Let me know your opinion. > > > > Best Regards, > > Hoeyonjiki Kim > > > >> > >>> > >>> > >>> Best Regards, > >>> Jaehoon Chung > >>> > >>>> break; > >>>> } > >>>> > >>>> > >>> > > >
[PATCH] env: mmc: Correct partition comparison in mmc_offset_try_partition
The function mmc_offset_try_partition searches MMC partition to save the environment data by name. However, it only compares the first word-size bytes (size of 'const char *'), which may make the function to find unintended partition. Correct the function not to partially compare the partition name with config "u-boot,,mmc-env-partition". Signed-off-by: Hoyeonjiki Kim --- env/mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/env/mmc.c b/env/mmc.c index 4e67180b23..505f7aa2b8 100644 --- a/env/mmc.c +++ b/env/mmc.c @@ -42,7 +42,7 @@ static inline int mmc_offset_try_partition(const char *str, int copy, s64 *val) if (ret < 0) return ret; - if (!strncmp((const char *)info.name, str, sizeof(str))) + if (!strcmp((const char *)info.name, str)) break; } -- 2.25.1
