[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 at 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 at iopsys.eu>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at 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
>>