* [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.