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@denx.de In Nature there are neither rewards nor punishments, there are conse- quences. -- R.G. Ingersoll