* [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition @ 2020-11-12 13:12 Hoyeonjiki Kim 2020-11-12 20:07 ` Wolfgang Denk 0 siblings, 1 reply; 5+ messages in thread From: Hoyeonjiki Kim @ 2020-11-12 13:12 UTC (permalink / raw) To: u-boot 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 <jigi.kim@gmail.com> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> --- 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition 2020-11-12 13:12 [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition Hoyeonjiki Kim @ 2020-11-12 20:07 ` Wolfgang Denk 2020-11-13 4:03 ` Hoyeonjiki Kim 0 siblings, 1 reply; 5+ messages in thread From: Wolfgang Denk @ 2020-11-12 20:07 UTC (permalink / raw) To: u-boot Dear Hoyeonjiki Kim, In message <20201112131237.1239-1-jigi.kim@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 <jigi.kim@gmail.com> > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> > --- > 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!! 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: wd at denx.de In Nature there are neither rewards nor punishments, there are conse- quences. -- R.G. Ingersoll ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition 2020-11-12 20:07 ` Wolfgang Denk @ 2020-11-13 4:03 ` Hoyeonjiki Kim 2020-11-15 16:36 ` Wolfgang Denk 0 siblings, 1 reply; 5+ messages in thread From: Hoyeonjiki Kim @ 2020-11-13 4:03 UTC (permalink / raw) To: u-boot On Fri, Nov 13, 2020 at 5:07 AM Wolfgang Denk <wd@denx.de> wrote: > > Dear Hoyeonjiki Kim, > > In message <20201112131237.1239-1-jigi.kim@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 <jigi.kim@gmail.com> > > Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com> > > --- > > 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: wd at denx.de > In Nature there are neither rewards nor punishments, there are conse- > quences. -- R.G. Ingersoll ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition 2020-11-13 4:03 ` Hoyeonjiki Kim @ 2020-11-15 16:36 ` Wolfgang Denk 2020-11-15 17:01 ` Hoyeonjiki Kim 0 siblings, 1 reply; 5+ messages in thread From: Wolfgang Denk @ 2020-11-15 16:36 UTC (permalink / raw) To: u-boot Dear Hoyeonjiki Kim, In message <CAL9K-_jni+850m5Zf7QPPeM+dmSxqSZ5AgirDnzE_2uxrO4Akg@mail.gmail.com> 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). > 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... 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: wd at denx.de It's not what you do, it's how you do what you do! - Jordan D. Ulmer ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition 2020-11-15 16:36 ` Wolfgang Denk @ 2020-11-15 17:01 ` Hoyeonjiki Kim 0 siblings, 0 replies; 5+ messages in thread From: Hoyeonjiki Kim @ 2020-11-15 17:01 UTC (permalink / raw) To: u-boot Dear Wolfgang Denk, On Mon, Nov 16, 2020 at 1:36 AM Wolfgang Denk <wd@denx.de> wrote: > > Dear Hoyeonjiki Kim, > > In message <CAL9K-_jni+850m5Zf7QPPeM+dmSxqSZ5AgirDnzE_2uxrO4Akg@mail.gmail.com> 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: wd at denx.de > It's not what you do, it's how you do what you do! - Jordan D. Ulmer ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-11-15 17:01 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-12 13:12 [PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition Hoyeonjiki Kim 2020-11-12 20:07 ` Wolfgang Denk 2020-11-13 4:03 ` Hoyeonjiki Kim 2020-11-15 16:36 ` Wolfgang Denk 2020-11-15 17:01 ` Hoyeonjiki Kim
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.