[PATCH v4 1/2] fs: semihosting: Fixup smh_read against overflow

[Sean Anderson]

On 10/14/25 12:03, Andrew Goodbody wrote:
> The sys call SYS_READ was being used in a way that did not allow for
> file sizes > 2^31. Fixing this required changing the signature of the
> function to allow file sizes up to 2^32 - 2 and also being able to
> return errors that could be tested for.
> 
> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
> ---
>  common/spl/spl_semihosting.c        |  7 ++++---
>  drivers/serial/serial_semihosting.c |  6 ++++--
>  fs/semihostingfs.c                  |  9 +++++----
>  include/semihosting.h               |  8 ++++----
>  lib/semihosting.c                   | 22 ++++++++++++++++++----
>  5 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
> index f36863fe48d25859ee9af67a67140893b6e9e262..cff6f5351e265136b3effe483fd5d641fdd109f8 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,
> 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..15f58d578b65f7dbbadce19093546f5bdb46c0c3 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)
> @@ -43,10 +44,10 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer,
>  		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..42832d86c711ff04e91b6b739c80629253f6d30a 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
> diff --git a/lib/semihosting.c b/lib/semihosting.c
> index 9be5bffd30124777c4f8110065e6cca5640312ac..e49b16b0aa34ab04b6c8949a3e7cab539fdbef82 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,23 @@ 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 if ((long)ret == -1) {
> +		*read_len = 0;
>  		return smh_errno();
> -	return len - ret;
> +	} else {
> +		/* Partial success */
> +		*read_len = len - ret;
> +		return 0;
> +	}
> +

There is no point. When are you going to read 3 GiB of file all at once on a 32-bit system? Any
sane user will read multiple times into smaller buffers.

So the correct fix is to add

if (len > LONG_MAX)
  return -EOVERFLOW;

to the beginning.

--Sean