[PATCH] env: mmc: Correct partition comparison in mmc_offset_try_partition
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; }
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;}
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;}
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; }
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,
- "str" is shorter than "info.name", and
- 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; }
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,
- "str" is shorter than "info.name", and
- 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; }
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
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
participants (5)
-
Hoyeonjiki Kim -
Jaehoon Chung -
Jorge Ramirez-Ortiz, Gmail -
Wolfgang Denk -
김호연지기