[PATCH v2] env: mmc: Correct partition comparison in mmc_offset_try_partition

[Hoyeonjiki Kim]

On Fri, Nov 13, 2020 at 5:07 AM Wolfgang Denk <wd at denx.de> wrote:
>
> Dear Hoyeonjiki Kim,
>
> In message <20201112131237.1239-1-jigi.kim at 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 at gmail.com>
> > Reviewed-by: Jaehoon Chung <jh80.chung at 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