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

Jaehoon Chung jh80.chung at samsung.com
Mon Nov 16 04:06:27 CET 2020


Dear Wolfgang,

On 11/13/20 5:01 AM, Wolfgang Denk wrote:
> Dear Jaehoon Chung,
> 
> In message <21adc771-9660-da52-65c8-c2029de9a29e at samsung.com> you 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 <jigi.kim at gmail.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))
>>
>> Using "strlen(str)" is better than changing to strcmp.
>>
>> strncmp(..., ..., strlen(str))
> 
> Is either of this a good idea? I mean, if you pass in random data,
> this will run forever and eventually create undefined behaviour. We
> know the maximum size, so why not limit it to that, as strncmp() did?

Actually, i don' want to use strcmp. 
If my remember is correct, strcmp is already reported about having security hole.
I had commented one example for using to check length.
But i agreed it's not good idea. Thanks for pointing out!

Best Regards,
Jaehoon Chung

> 
> Best regards,
> 
> Wolfgang Denk
> 



More information about the U-Boot mailing list