* Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
2025-06-24 15:34 ` [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name() Heinrich Schuchardt
@ 2025-06-24 21:02 ` Mikhail Kshevetskiy
2025-06-24 23:05 ` Heinrich Schuchardt
2025-06-24 21:05 ` Mikhail Kshevetskiy
2025-06-25 2:18 ` E Shattow
2 siblings, 1 reply; 12+ messages in thread
From: Mikhail Kshevetskiy @ 2025-06-24 21:02 UTC (permalink / raw)
To: Heinrich Schuchardt, Tom Rini
Cc: Simon Glass, Sam Edwards, Anshul Dalal, u-boot
On 24.06.2025 18:34, Heinrich Schuchardt wrote:
> [You don't often get email from heinrich.schuchardt@canonical.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> A malformed FIT image could have an image name property that is not NUL
> terminated. Reject such images.
>
> Reported-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> common/spl/spl_fit.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index e250c11ebbd..25f3c822a49 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -73,7 +73,7 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
> const char **outname)
> {
> struct udevice *sysinfo;
> - const char *name, *str;
> + const char *name, *str, *end;
> __maybe_unused int node;
> int len, i;
> bool found = true;
> @@ -83,11 +83,17 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
> debug("cannot find property '%s': %d\n", type, len);
> return -EINVAL;
> }
> + /* A string property should be NUL terminated */
> + end = name + len - 1;
> + if (!len || *end) {
> + debug("malformed property '%s'\n", type);
> + return -EINVAL;
> + }
>
> str = name;
> for (i = 0; i < index; i++) {
> str = strchr(str, '\0') + 1;
The line above has a clear bug. str will never be NULL, so the check on
the next line is ineffective or just broken.
> - if (!str || (str - name >= len)) {
> + if (str > end) {
> found = false;
> break;
> }
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
2025-06-24 21:02 ` Mikhail Kshevetskiy
@ 2025-06-24 23:05 ` Heinrich Schuchardt
0 siblings, 0 replies; 12+ messages in thread
From: Heinrich Schuchardt @ 2025-06-24 23:05 UTC (permalink / raw)
To: Mikhail Kshevetskiy
Cc: Simon Glass, Sam Edwards, Anshul Dalal, u-boot, Tom Rini
On 24.06.25 23:02, Mikhail Kshevetskiy wrote:
>
> On 24.06.2025 18:34, Heinrich Schuchardt wrote:
>> [You don't often get email from heinrich.schuchardt@canonical.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> A malformed FIT image could have an image name property that is not NUL
>> terminated. Reject such images.
>>
>> Reported-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> common/spl/spl_fit.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index e250c11ebbd..25f3c822a49 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -73,7 +73,7 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>> const char **outname)
>> {
>> struct udevice *sysinfo;
>> - const char *name, *str;
>> + const char *name, *str, *end;
>> __maybe_unused int node;
>> int len, i;
>> bool found = true;
>> @@ -83,11 +83,17 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>> debug("cannot find property '%s': %d\n", type, len);
>> return -EINVAL;
>> }
>> + /* A string property should be NUL terminated */
>> + end = name + len - 1;
>> + if (!len || *end) {
>> + debug("malformed property '%s'\n", type);
>> + return -EINVAL;
>> + }
>>
>> str = name;
>> for (i = 0; i < index; i++) {
>> str = strchr(str, '\0') + 1;
> The line above has a clear bug. str will never be NULL, so the check on
> the next line is ineffective or just broken.
Yes, strchr() searching for NUL will never be NULL. But when sending
your follow up mail 3 minutes later you claim the opposite.
>> - if (!str || (str - name >= len)) {
Please, notice that this line is being replaced by the patch.
Best regards
Heinrich
>> + if (str > end) {
>> found = false;
>> break;
>> }
>> --
>> 2.48.1
>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
2025-06-24 15:34 ` [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name() Heinrich Schuchardt
2025-06-24 21:02 ` Mikhail Kshevetskiy
@ 2025-06-24 21:05 ` Mikhail Kshevetskiy
2025-06-24 23:05 ` Heinrich Schuchardt
2025-06-25 2:18 ` E Shattow
2 siblings, 1 reply; 12+ messages in thread
From: Mikhail Kshevetskiy @ 2025-06-24 21:05 UTC (permalink / raw)
To: Heinrich Schuchardt, Tom Rini
Cc: Simon Glass, Sam Edwards, Anshul Dalal, u-boot
On 24.06.2025 18:34, Heinrich Schuchardt wrote:
> [You don't often get email from heinrich.schuchardt@canonical.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> A malformed FIT image could have an image name property that is not NUL
> terminated. Reject such images.
>
> Reported-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> common/spl/spl_fit.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index e250c11ebbd..25f3c822a49 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -73,7 +73,7 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
> const char **outname)
> {
> struct udevice *sysinfo;
> - const char *name, *str;
> + const char *name, *str, *end;
> __maybe_unused int node;
> int len, i;
> bool found = true;
> @@ -83,11 +83,17 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
> debug("cannot find property '%s': %d\n", type, len);
> return -EINVAL;
> }
> + /* A string property should be NUL terminated */
> + end = name + len - 1;
> + if (!len || *end) {
> + debug("malformed property '%s'\n", type);
> + return -EINVAL;
> + }
>
> str = name;
> for (i = 0; i < index; i++) {
> str = strchr(str, '\0') + 1;
> - if (!str || (str - name >= len)) {
> + if (str > end) {
and if strchr() will return NULL, then str will be equal to 1. In this
case str will be less then end, so the loop will not terminate.
> found = false;
> break;
> }
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
2025-06-24 21:05 ` Mikhail Kshevetskiy
@ 2025-06-24 23:05 ` Heinrich Schuchardt
2025-06-24 23:07 ` Mikhail Kshevetskiy
0 siblings, 1 reply; 12+ messages in thread
From: Heinrich Schuchardt @ 2025-06-24 23:05 UTC (permalink / raw)
To: Mikhail Kshevetskiy
Cc: Simon Glass, Sam Edwards, Anshul Dalal, u-boot, Tom Rini
On 24.06.25 23:05, Mikhail Kshevetskiy wrote:
>
> On 24.06.2025 18:34, Heinrich Schuchardt wrote:
>> [You don't often get email from heinrich.schuchardt@canonical.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>>
>> A malformed FIT image could have an image name property that is not NUL
>> terminated. Reject such images.
>>
>> Reported-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> common/spl/spl_fit.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index e250c11ebbd..25f3c822a49 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -73,7 +73,7 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>> const char **outname)
>> {
>> struct udevice *sysinfo;
>> - const char *name, *str;
>> + const char *name, *str, *end;
>> __maybe_unused int node;
>> int len, i;
>> bool found = true;
>> @@ -83,11 +83,17 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>> debug("cannot find property '%s': %d\n", type, len);
>> return -EINVAL;
>> }
>> + /* A string property should be NUL terminated */
>> + end = name + len - 1;
>> + if (!len || *end) {
>> + debug("malformed property '%s'\n", type);
>> + return -EINVAL;
>> + }
>>
>> str = name;
>> for (i = 0; i < index; i++) {
>> str = strchr(str, '\0') + 1;
>> - if (!str || (str - name >= len)) {
>> + if (str > end) {
>
> and if strchr() will return NULL, then str will be equal to 1. In this
> case str will be less then end, so the loop will not terminate.
strchr() searching for NUL will never return NULL but search until it
hits NUL.
And as the patch adds checks that the buffer pointed to by str is NUL
terminated we will not read outside of the buffer.
Best regards
Heinrich
>
>
>> found = false;
>> break;
>> }
>> --
>> 2.48.1
>>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
2025-06-24 23:05 ` Heinrich Schuchardt
@ 2025-06-24 23:07 ` Mikhail Kshevetskiy
0 siblings, 0 replies; 12+ messages in thread
From: Mikhail Kshevetskiy @ 2025-06-24 23:07 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Simon Glass, Sam Edwards, Anshul Dalal, u-boot, Tom Rini
On 25.06.2025 02:05, Heinrich Schuchardt wrote:
> [You don't often get email from heinrich.schuchardt@canonical.com.
> Learn why this is important at
> https://aka.ms/LearnAboutSenderIdentification ]
>
> On 24.06.25 23:05, Mikhail Kshevetskiy wrote:
>>
>> On 24.06.2025 18:34, Heinrich Schuchardt wrote:
>>> [You don't often get email from heinrich.schuchardt@canonical.com.
>>> Learn why this is important at
>>> https://aka.ms/LearnAboutSenderIdentification ]
>>>
>>> A malformed FIT image could have an image name property that is not NUL
>>> terminated. Reject such images.
>>>
>>> Reported-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> ---
>>> common/spl/spl_fit.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index e250c11ebbd..25f3c822a49 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -73,7 +73,7 @@ static int spl_fit_get_image_name(const struct
>>> spl_fit_info *ctx,
>>> const char **outname)
>>> {
>>> struct udevice *sysinfo;
>>> - const char *name, *str;
>>> + const char *name, *str, *end;
>>> __maybe_unused int node;
>>> int len, i;
>>> bool found = true;
>>> @@ -83,11 +83,17 @@ static int spl_fit_get_image_name(const struct
>>> spl_fit_info *ctx,
>>> debug("cannot find property '%s': %d\n", type, len);
>>> return -EINVAL;
>>> }
>>> + /* A string property should be NUL terminated */
>>> + end = name + len - 1;
>>> + if (!len || *end) {
>>> + debug("malformed property '%s'\n", type);
>>> + return -EINVAL;
>>> + }
>>>
>>> str = name;
>>> for (i = 0; i < index; i++) {
>>> str = strchr(str, '\0') + 1;
>>> - if (!str || (str - name >= len)) {
>>> + if (str > end) {
>>
>> and if strchr() will return NULL, then str will be equal to 1. In this
>> case str will be less then end, so the loop will not terminate.
>
> strchr() searching for NUL will never return NULL but search until it
> hits NUL.
>
> And as the patch adds checks that the buffer pointed to by str is NUL
> terminated we will not read outside of the buffer.
Ok, good from my side
>
> Best regards
>
> Heinrich
>
>>
>>
>>> found = false;
>>> break;
>>> }
>>> --
>>> 2.48.1
>>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
2025-06-24 15:34 ` [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name() Heinrich Schuchardt
2025-06-24 21:02 ` Mikhail Kshevetskiy
2025-06-24 21:05 ` Mikhail Kshevetskiy
@ 2025-06-25 2:18 ` E Shattow
2 siblings, 0 replies; 12+ messages in thread
From: E Shattow @ 2025-06-25 2:18 UTC (permalink / raw)
To: Heinrich Schuchardt, Tom Rini
Cc: Simon Glass, Mikhail Kshevetskiy, Sam Edwards, Anshul Dalal,
u-boot
On 6/24/25 08:34, Heinrich Schuchardt wrote:
> A malformed FIT image could have an image name property that is not NUL
> terminated. Reject such images.
>
> Reported-by: Mikhail Kshevetskiy <mikhail.kshevetskiy@iopsys.eu>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> common/spl/spl_fit.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index e250c11ebbd..25f3c822a49 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -73,7 +73,7 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
> const char **outname)
> {
> struct udevice *sysinfo;
> - const char *name, *str;
> + const char *name, *str, *end;
> __maybe_unused int node;
> int len, i;
> bool found = true;
> @@ -83,11 +83,17 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
> debug("cannot find property '%s': %d\n", type, len);
> return -EINVAL;
> }
> + /* A string property should be NUL terminated */
> + end = name + len - 1;
> + if (!len || *end) {
> + debug("malformed property '%s'\n", type);
> + return -EINVAL;
> + }
>
> str = name;
> for (i = 0; i < index; i++) {
> str = strchr(str, '\0') + 1;
> - if (!str || (str - name >= len)) {
> + if (str > end) {
> found = false;
> break;
> }
Tested-by: E Shattow <e@freeshell.de>
^ permalink raw reply [flat|nested] 12+ messages in thread