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

[Heinrich Schuchardt <heinrich.schuchardt@canonical.com>]

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