[PATCH 0/2] spl: fix error handling in spl_fit_get_image_name()

[PATCH 0/2] spl: fix error handling in spl_fit_get_image_name()

[Heinrich Schuchardt]

spl_fit_get_image_name() used to lack a detection of malformed image name
properties in FIT images. The change in commit 3704b888a4ca ("common/spl:
fix potential out of buffer access in spl_fit_get_image_name function")
tried to fix this  but led to function spl_fit_get_image_name() no longer
detecting if a property at index > 1 does not exist.

This patch is reverted.

An explicit check for malformed image name properties is introduced.

Cf.
[BUG] [PATCH v5 1/3] common/spl: fix potential out of buffer access in
spl_fit_get_image_name function
https://lore.kernel.org/u-boot/38f5d078-3328-4bdb-9c95-4fb5fe89ddc2@gmx.de/T/#u

Heinrich Schuchardt (2):
  common/spl: Revert fix potential out of buffer access in
    spl_fit_get_image_name function
  common/spl: guard against buffer overflow in spl_fit_get_image_name()

 common/spl/spl_fit.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.48.1

[PATCH 1/2] common/spl: Revert fix potential out of buffer access in spl_fit_get_image_name function

[Heinrich Schuchardt]

The change in commit 3704b888a4ca ("common/spl: fix potential out of buffer
access in spl_fit_get_image_name function") led to function
spl_fit_get_image_name() no longer detecting if a property does not exist
at a non-zero buffer.

Link: https://lore.kernel.org/u-boot/38f5d078-3328-4bdb-9c95-4fb5fe89ddc2@gmx.de/T/#m59f3a23e675daa992c28d12236de71cae2ca2bb9
Fixes: 3704b888a4ca ("common/spl: fix potential out of buffer access in spl_fit_get_image_name function")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 common/spl/spl_fit.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
index b3824af475f..e250c11ebbd 100644
--- a/common/spl/spl_fit.c
+++ b/common/spl/spl_fit.c
@@ -86,12 +86,11 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
 
 	str = name;
 	for (i = 0; i < index; i++) {
-		str = memchr(str, '\0', name + len - str);
-		if (!str) {
+		str = strchr(str, '\0') + 1;
+		if (!str || (str - name >= len)) {
 			found = false;
 			break;
 		}
-		str++;
 	}
 
 	if (!found && CONFIG_IS_ENABLED(SYSINFO) && !sysinfo_get(&sysinfo)) {
-- 
2.48.1

[PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[Heinrich Schuchardt]

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;
 		}
-- 
2.48.1

Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[Mikhail Kshevetskiy]

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
>

Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[Mikhail Kshevetskiy]

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
>

Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[Heinrich Schuchardt]

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

Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[Heinrich Schuchardt]

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

Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[Mikhail Kshevetskiy]

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

Re: [PATCH 0/2] spl: fix error handling in spl_fit_get_image_name()

[E Shattow]

Hi Heinrich,

On 6/24/25 08:34, Heinrich Schuchardt wrote:
> spl_fit_get_image_name() used to lack a detection of malformed image name
> properties in FIT images. The change in commit 3704b888a4ca ("common/spl:
> fix potential out of buffer access in spl_fit_get_image_name function")
> tried to fix this  but led to function spl_fit_get_image_name() no longer
> detecting if a property at index > 1 does not exist.
> 
> This patch is reverted.
> 
> An explicit check for malformed image name properties is introduced.
> 
> Cf.
> [BUG] [PATCH v5 1/3] common/spl: fix potential out of buffer access in
> spl_fit_get_image_name function
> https://lore.kernel.org/u-boot/38f5d078-3328-4bdb-9c95-4fb5fe89ddc2@gmx.de/T/#u
> 
> Heinrich Schuchardt (2):
>   common/spl: Revert fix potential out of buffer access in
>     spl_fit_get_image_name function
>   common/spl: guard against buffer overflow in spl_fit_get_image_name()
> 
>  common/spl/spl_fit.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 

Before:
U-Boot SPL 2025.07-rc4-00030-g0daf2d0cabef (Jun 22 2025 - 23:27:44 -0700)
DDR version: dc2e84f0.
Trying to boot from SPI
cannot find image node '': -1

OpenSBI v1.6

After:
U-Boot SPL 2025.07-rc4-00032-g562b7d099d46 (Jun 24 2025 - 18:53:15 -0700)
DDR version: dc2e84f0.
Trying to boot from SPI

OpenSBI v1.6

For the series,

Tested-By: E Shattow <e@freeshell.de>

Re: [PATCH 1/2] common/spl: Revert fix potential out of buffer access in spl_fit_get_image_name function

[E Shattow]

On 6/24/25 08:34, Heinrich Schuchardt wrote:
> The change in commit 3704b888a4ca ("common/spl: fix potential out of buffer
> access in spl_fit_get_image_name function") led to function
> spl_fit_get_image_name() no longer detecting if a property does not exist
> at a non-zero buffer.
> 
> Link: https://lore.kernel.org/u-boot/38f5d078-3328-4bdb-9c95-4fb5fe89ddc2@gmx.de/T/#m59f3a23e675daa992c28d12236de71cae2ca2bb9
> Fixes: 3704b888a4ca ("common/spl: fix potential out of buffer access in spl_fit_get_image_name function")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>  common/spl/spl_fit.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
> index b3824af475f..e250c11ebbd 100644
> --- a/common/spl/spl_fit.c
> +++ b/common/spl/spl_fit.c
> @@ -86,12 +86,11 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>  
>  	str = name;
>  	for (i = 0; i < index; i++) {
> -		str = memchr(str, '\0', name + len - str);
> -		if (!str) {
> +		str = strchr(str, '\0') + 1;
> +		if (!str || (str - name >= len)) {
>  			found = false;
>  			break;
>  		}
> -		str++;
>  	}
>  
>  	if (!found && CONFIG_IS_ENABLED(SYSINFO) && !sysinfo_get(&sysinfo)) {

Tested-by: E Shattow <e@freeshell.de>

Re: [PATCH 2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()

[E Shattow]

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>

Re: [PATCH 0/2] spl: fix error handling in spl_fit_get_image_name()

[Tom Rini]

On Tue, 24 Jun 2025 17:34:29 +0200, Heinrich Schuchardt wrote:

> spl_fit_get_image_name() used to lack a detection of malformed image name
> properties in FIT images. The change in commit 3704b888a4ca ("common/spl:
> fix potential out of buffer access in spl_fit_get_image_name function")
> tried to fix this  but led to function spl_fit_get_image_name() no longer
> detecting if a property at index > 1 does not exist.
> 
> This patch is reverted.
> 
> [...]

Applied to u-boot/master, thanks!

[1/2] common/spl: Revert fix potential out of buffer access in spl_fit_get_image_name function
      commit: 6ef9a89c1da433108ac89deab58954c6e2b798ba
[2/2] common/spl: guard against buffer overflow in spl_fit_get_image_name()
      commit: 79f8f31d58dfcd2b3563c32f1cf1097cee4d7f76
-- 
Tom