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

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

[Hoyeonjiki Kim]

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@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))
 			break;
 	}
 
-- 
2.25.1

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

[Jaehoon Chung]

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@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))


Best Regards,
Jaehoon Chung

>  			break;
>  	}
>  
> 

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

[Jorge]

On 11/11/20, Jaehoon Chung 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@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))

absolutely.
maybe also modify the commit like to indicate a fix to the current bug?

> 
> 
> Best Regards,
> Jaehoon Chung
> 
> >  			break;
> >  	}
> >  
> > 
> 

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

[김호연지기]

On Wed, Nov 11, 2020 at 7:07 PM Jorge Ramirez-Ortiz, Gmail
<jorge.ramirez.ortiz@gmail.com> wrote:
>
> On 11/11/20, Jaehoon Chung 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@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))
>
> absolutely.
> maybe also modify the commit like to indicate a fix to the current bug?

Thanks for the feedback for both.

However, when we do so, isn't there still the possibility for the
function searching incorrect partition if,
1) "str" is shorter than "info.name", and
2) the first "strlen(str)" letters of "info.name" is same with those of "str"?

This commit is for fixing the current bug, but also I wanna make sure
that partition name matches fully.

Let me know your opinion.

Best Regards,
Hoeyonjiki Kim

>
> >
> >
> > Best Regards,
> > Jaehoon Chung
> >
> > >                     break;
> > >     }
> > >
> > >
> >

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

[Jaehoon Chung]

Hi,

On 11/11/20 7:25 PM, ????? wrote:
> On Wed, Nov 11, 2020 at 7:07 PM Jorge Ramirez-Ortiz, Gmail
> <jorge.ramirez.ortiz@gmail.com> wrote:
>>
>> On 11/11/20, Jaehoon Chung 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@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))
>>
>> absolutely.
>> maybe also modify the commit like to indicate a fix to the current bug?
> 
> Thanks for the feedback for both.
> 
> However, when we do so, isn't there still the possibility for the
> function searching incorrect partition if,
> 1) "str" is shorter than "info.name", and
> 2) the first "strlen(str)" letters of "info.name" is same with those of "str"?

Make sense. You're right. :)

There are two approach. One is that choose one of those length what is longer.
Other is your approach. I don't have any objection to fix whatever.

Just resend your patch with Jorge's comment, plz. 


Best Regards,
Jaehoon Chung

> 
> This commit is for fixing the current bug, but also I wanna make sure
> that partition name matches fully.
> 
> Let me know your opinion.
> 
> Best Regards,
> Hoeyonjiki Kim
> 
>>
>>>
>>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>>                     break;
>>>>     }
>>>>
>>>>
>>>
> 

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

[Hoyeonjiki Kim]

On Thu, Nov 12, 2020 at 8:34 AM Jaehoon Chung <jh80.chung@samsung.com> wrote:
>
> Hi,
>
> On 11/11/20 7:25 PM, ????? wrote:
> > On Wed, Nov 11, 2020 at 7:07 PM Jorge Ramirez-Ortiz, Gmail
> > <jorge.ramirez.ortiz@gmail.com> wrote:
> >>
> >> On 11/11/20, Jaehoon Chung 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@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))
> >>
> >> absolutely.
> >> maybe also modify the commit like to indicate a fix to the current bug?
> >
> > Thanks for the feedback for both.
> >
> > However, when we do so, isn't there still the possibility for the
> > function searching incorrect partition if,
> > 1) "str" is shorter than "info.name", and
> > 2) the first "strlen(str)" letters of "info.name" is same with those of "str"?
>
> Make sense. You're right. :)
>
> There are two approach. One is that choose one of those length what is longer.
> Other is your approach. I don't have any objection to fix whatever.
>
> Just resend your patch with Jorge's comment, plz.
>
>
> Best Regards,
> Jaehoon Chung

Sure, thanks for your opinion.
Will be back with v2.

Best Regards,
Hoyeonjiki Kim

>
> >
> > This commit is for fixing the current bug, but also I wanna make sure
> > that partition name matches fully.
> >
> > Let me know your opinion.
> >
> > Best Regards,
> > Hoeyonjiki Kim
> >
> >>
> >>>
> >>>
> >>> Best Regards,
> >>> Jaehoon Chung
> >>>
> >>>>                     break;
> >>>>     }
> >>>>
> >>>>
> >>>
> >
>

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

[Wolfgang Denk]

Dear Jaehoon Chung,

In message <21adc771-9660-da52-65c8-c2029de9a29e@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@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?

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
"To IBM, 'open' means there is a modicum  of  interoperability  among
some of their equipment."                            - Harv Masterson

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

[Jaehoon Chung]

Dear Wolfgang,

On 11/13/20 5:01 AM, Wolfgang Denk wrote:
> Dear Jaehoon Chung,
> 
> In message <21adc771-9660-da52-65c8-c2029de9a29e@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@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
> 

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

[Hoyeonjiki Kim]

On Fri, Nov 13, 2020 at 5:06 AM Wolfgang Denk <wd@denx.de> wrote:
>
> Dear Hoyeonjiki Kim,
>
> In message <20201112123026.458-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))
>
> This looks dangerous to me.  If the used length  sizeof(str) is not
> correct, that should be fixed, but switching to strcmp() means we
> may run into problems if the buffer is not NUL terminated.  Do we
> have a guarantee here thai it always ist??  The fact that strncmp()
> has been used before suggests otyherwise.
>
> 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
> Computers are not intelligent. They only think they are.

Dear Wolfgang Denk,

I left my reply on the thread for patch v2.
Please refer to that.

We can keep discussing it on that thread from now on.
Again, sorry for the confusion.

Best Regards,
Hoyeonjiki Kim

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

[Wolfgang Denk]

Dear Hoyeonjiki Kim,

In message <20201112123026.458-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))

This looks dangerous to me.  If the used length  sizeof(str) is not
correct, that should be fixed, but switching to strcmp() means we
may run into problems if the buffer is not NUL terminated.  Do we
have a guarantee here thai it always ist??  The fact that strncmp()
has been used before suggests otyherwise.

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
Computers are not intelligent. They only think they are.

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

[Hoyeonjiki Kim]

Mis-submitted the patch without versioning... :(
I'll re-submit my patch again.

Sorry for interrupting you.

Best Regards,
Hoyeonjiki Kim

On Thu, Nov 12, 2020 at 9:30 PM Hoyeonjiki Kim <jigi.kim@gmail.com> 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))
>                         break;
>         }
>
> --
> 2.25.1
>

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

[Hoyeonjiki Kim]

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