[PATCH v2] fs: semihosting: Fixup smh_flen and smh_read against overflow

Andrew Goodbody andrew.goodbody at linaro.org
Fri Oct 3 10:56:19 CEST 2025


On 03/10/2025 06:31, Heinrich Schuchardt wrote:
> On 10/2/25 16:32, Andrew Goodbody wrote:
>> The sys calls SYS_FLEN and SYS_READ were being used in a way that did
>> not allow for file sizes > 2^31. Fixing this required changing the
>> signatures of the functions smh_flen and smh_read to allow file
>> sizes up to 2^32 - 2 and also being able to return errors that could be
>> tested for.
>>
>> This issue was found by Smatch.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
> 
> I applied this patch to origin/master and build qemu_arm_defconfig with 
> CONFIG_SEMIHOSTING=y. It seems you missed one occurence of smh_flen.
> 
> fs/semihostingfs.c: In function ‘smh_fs_size’:
> fs/semihostingfs.c:88:16: error: too few arguments to function 
> ‘smh_flen’; expected 2, have 1
>     88 |         size = smh_flen(fd);
>        |                ^~~~~~~~
> In file included from fs/semihostingfs.c:11:
> include/semihosting.h:131:5: note: declared here
>    131 | int smh_flen(long fd, size_t *size);
>        |     ^~~~~~~~
> 
> To fix the issue I applied this diff on top of your patch:
> 
> diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c
> index 71b6634b066..d2f065782d6 100644
> --- a/fs/semihostingfs.c
> +++ b/fs/semihostingfs.c
> @@ -79,17 +79,19 @@ static int smh_fs_write_at(const char *filename, 
> loff_t pos, void *buffer,
> 
>   int smh_fs_size(const char *filename, loff_t *result)
>   {
> -       long fd, size;
> +       long fd;
> +       size_t size;
> +       int ret;
> 
>          fd = smh_open(filename, MODE_READ | MODE_BINARY);
>          if (fd < 0)
>                  return fd;
> 
> -       size = smh_flen(fd);
> +       ret = smh_flen(fd, &size);
>          smh_close(fd);
> 
> -       if (size < 0)
> -               return size;
> +       if (ret)
> +               return ret;
> 
>          *result = size;
>          return 0;
> 
> With that correction the size command gives me these results for files 
> with size 3 GiB and 5 GiB on 32bit ARM:
> 
> => size hostfs - 3G ; echo $filesize
> c0000000
> => size hostfs - 5G ; echo $filesize
> 40000000
> 
> The incorrect result for 5G needs to be corrected in QEMU. QEMU should 
> return an error for files exceeding 0xfffffffe length on 32 bit systems.
> 
> On 64bit ARM everything is fine:
> 
> => size hostfs - 3 ; echo $filesize
> c0000000
> => size hostfs - 5 ; echo $filesize
> 140000000
> 
> For a missing file the error is correctly reported:
> => size hostfs - 2; echo $?
> 1
> 
> For a file without access writes $? is also set to $1 because opening 
> already fails:
> 
> $ ls r
> -r-------- 1 root root 10 Okt  3 07:25 r
> 
> => size hostfs - r; echo $?
> 1
> 
> Please, resubmit the patch.

Sorry, yes, will do.

Thanks,
Andrew

> Best regards
> 
> Heinrich
> 
>> ---
>> Changes in v2:
>> - Fixup smh_flen and smh_read to allow for > 2^31 byte files
>> - Link to v1: https://lore.kernel.org/r/20251002-fs_semihosting- 
>> v1-1-1124e6becc51 at linaro.org
>> ---
>>   common/spl/spl_semihosting.c        | 13 +++++++------
>>   drivers/serial/serial_semihosting.c |  6 ++++--
>>   fs/semihostingfs.c                  | 13 +++++++------
>>   include/semihosting.h               | 13 +++++++------
>>   lib/semihosting.c                   | 25 +++++++++++++++++++------
>>   5 files changed, 44 insertions(+), 26 deletions(-)
>>
>> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
>> index 
>> f36863fe48d25859ee9af67a67140893b6e9e262..fb578a274d0d937b2ed5bd13e505bcefd4491799 100644
>> --- a/common/spl/spl_semihosting.c
>> +++ b/common/spl/spl_semihosting.c
>> @@ -13,13 +13,14 @@ static ulong smh_fit_read(struct spl_load_info 
>> *load, ulong file_offset,
>>                 ulong size, void *buf)
>>   {
>>       long fd = *(long *)load->priv;
>> -    ulong ret;
>> +    long ret;
>> +    size_t bytes_read;
>>       if (smh_seek(fd, file_offset))
>>           return 0;
>> -    ret = smh_read(fd, buf, size);
>> -    return ret < 0 ? 0 : ret;
>> +    ret = smh_read(fd, buf, size, &bytes_read);
>> +    return ret < 0 ? 0 : bytes_read;
>>   }
>>   static int spl_smh_load_image(struct spl_image_info *spl_image,
>> @@ -27,7 +28,8 @@ static int spl_smh_load_image(struct spl_image_info 
>> *spl_image,
>>   {
>>       const char *filename = CONFIG_SPL_FS_LOAD_PAYLOAD_NAME;
>>       int ret;
>> -    long fd, len;
>> +    long fd;
>> +    size_t len;
>>       struct spl_load_info load;
>>       fd = smh_open(filename, MODE_READ | MODE_BINARY);
>> @@ -36,12 +38,11 @@ static int spl_smh_load_image(struct 
>> spl_image_info *spl_image,
>>           return fd;
>>       }
>> -    ret = smh_flen(fd);
>> +    ret = smh_flen(fd, &len);
>>       if (ret < 0) {
>>           log_debug("could not get length of image: %d\n", ret);
>>           goto out;
>>       }
>> -    len = ret;
>>       spl_load_init(&load, smh_fit_read, &fd, 1);
>>       ret = spl_load(spl_image, bootdev, &load, len, 0);
>> diff --git a/drivers/serial/serial_semihosting.c b/drivers/serial/ 
>> serial_semihosting.c
>> index 
>> 56a5ec72428afdd97eccad739a46189380ba3e76..017b1f8eda84f5c25e09be7e970d0ebfdbf2ace0 100644
>> --- a/drivers/serial/serial_semihosting.c
>> +++ b/drivers/serial/serial_semihosting.c
>> @@ -25,11 +25,12 @@ static int smh_serial_getc(struct udevice *dev)
>>   {
>>       char ch = 0;
>>       struct smh_serial_priv *priv = dev_get_priv(dev);
>> +    size_t bytes_read;
>>       if (priv->infd < 0)
>>           return smh_getc();
>> -    smh_read(priv->infd, &ch, sizeof(ch));
>> +    smh_read(priv->infd, &ch, sizeof(ch), &bytes_read);
>>       return ch;
>>   }
>> @@ -140,11 +141,12 @@ static void smh_serial_setbrg(void)
>>   static int smh_serial_getc(void)
>>   {
>>       char ch = 0;
>> +    size_t bytes_read;
>>       if (infd < 0)
>>           return smh_getc();
>> -    smh_read(infd, &ch, sizeof(ch));
>> +    smh_read(infd, &ch, sizeof(ch), &bytes_read);
>>       return ch;
>>   }
>> diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c
>> index 
>> 77e39ca407e4d240a1fd573497c5b6b908816454..71b6634b0660e32cc63f7fc60ec1af8d3248433a 100644
>> --- a/fs/semihostingfs.c
>> +++ b/fs/semihostingfs.c
>> @@ -23,7 +23,8 @@ int smh_fs_set_blk_dev(struct blk_desc *rbdd, struct 
>> disk_partition *info)
>>   static int smh_fs_read_at(const char *filename, loff_t pos, void 
>> *buffer,
>>                 loff_t maxsize, loff_t *actread)
>>   {
>> -    long fd, size, ret;
>> +    long fd, ret;
>> +    size_t size;
>>       fd = smh_open(filename, MODE_READ | MODE_BINARY);
>>       if (fd < 0)
>> @@ -34,19 +35,19 @@ static int smh_fs_read_at(const char *filename, 
>> loff_t pos, void *buffer,
>>           return ret;
>>       }
>>       if (!maxsize) {
>> -        size = smh_flen(fd);
>> +        ret = smh_flen(fd, &size);
>>           if (ret < 0) {
>>               smh_close(fd);
>> -            return size;
>> +            return ret;
>>           }
>>           maxsize = size;
>>       }
>> -    size = smh_read(fd, buffer, maxsize);
>> +    ret = smh_read(fd, buffer, maxsize, &size);
>>       smh_close(fd);
>> -    if (size < 0)
>> -        return size;
>> +    if (ret < 0)
>> +        return ret;
>>       *actread = size;
>>       return 0;
>> diff --git a/include/semihosting.h b/include/semihosting.h
>> index 
>> 4e844cbad87bb1ae6bb365f87f3e7a8aeea445f4..60535199b9532ce16620aea3ca1ea741c5b73766 100644
>> --- a/include/semihosting.h
>> +++ b/include/semihosting.h
>> @@ -95,12 +95,12 @@ long smh_open(const char *fname, enum 
>> smh_open_mode mode);
>>    * @fd: A file descriptor returned from smh_open()
>>    * @memp: Pointer to a buffer of memory of at least @len bytes
>>    * @len: The number of bytes to read
>> + * @read_len: Pointer which will be updated with actual bytes read on 
>> success,
>> + *  with 0 indicating %EOF
>>    *
>> - * Return:
>> - * * The number of bytes read on success, with 0 indicating %EOF
>> - * * A negative error on failure
>> + * Return: 0 on success or negative error on failure
>>    */
>> -long smh_read(long fd, void *memp, size_t len);
>> +long smh_read(long fd, void *memp, size_t len, size_t *read_len);
>>   /**
>>    * smh_write() - Write data to a file
>> @@ -124,10 +124,11 @@ long smh_close(long fd);
>>   /**
>>    * smh_flen() - Get the length of a file
>>    * @fd: A file descriptor returned from smh_open()
>> + * @size: Pointer which will be updated with length of file on success
>>    *
>> - * Return: The length of the file, in bytes, or a negative error on 
>> failure
>> + * Return: 0 on success or negative error on failure
>>    */
>> -long smh_flen(long fd);
>> +int smh_flen(long fd, size_t *size);
>>   /**
>>    * smh_seek() - Seek to a position in a file
>> diff --git a/lib/semihosting.c b/lib/semihosting.c
>> index 
>> 9be5bffd30124777c4f8110065e6cca5640312ac..a0f42d41e4a7ad95023c684f6824bfc677ad25cd 100644
>> --- a/lib/semihosting.c
>> +++ b/lib/semihosting.c
>> @@ -93,9 +93,9 @@ struct smh_rdwr_s {
>>       size_t len;
>>   };
>> -long smh_read(long fd, void *memp, size_t len)
>> +long smh_read(long fd, void *memp, size_t len, size_t *read_len)
>>   {
>> -    long ret;
>> +    size_t ret;
>>       struct smh_rdwr_s read;
>>       debug("%s: fd %ld, memp %p, len %zu\n", __func__, fd, memp, len);
>> @@ -105,9 +105,20 @@ long smh_read(long fd, void *memp, size_t len)
>>       read.len = len;
>>       ret = smh_trap(SYSREAD, &read);
>> -    if (ret < 0)
>> +    if (!ret) {
>> +        /* Success */
>> +        *read_len = len;
>> +        return 0;
>> +    } else if (ret == len) {
>> +        /* EOF */
>> +        *read_len = 0;
>> +        return 0;
>> +    } else {
>> +        /* Partial success */
>> +        *read_len = len - ret;
>>           return smh_errno();
>> -    return len - ret;
>> +    }
>> +
>>   }
>>   long smh_write(long fd, const void *memp, size_t len, ulong *written)
>> @@ -140,7 +151,7 @@ long smh_close(long fd)
>>       return 0;
>>   }
>> -long smh_flen(long fd)
>> +int smh_flen(long fd, size_t *size)
>>   {
>>       long ret;
>> @@ -149,7 +160,9 @@ long smh_flen(long fd)
>>       ret = smh_trap(SYSFLEN, &fd);
>>       if (ret == -1)
>>           return smh_errno();
>> -    return ret;
>> +
>> +    *size = (size_t)ret;
>> +    return 0;
>>   }
>>   long smh_seek(long fd, long pos)
>>
>> ---
>> base-commit: da47ddebd16a7e1047da8537fbf01558d2a89fcf
>> change-id: 20251002-fs_semihosting-85d697fbfcad
>>
>> Best regards,
> 



More information about the U-Boot mailing list