On 02/10/2025 16:42, Sean Anderson wrote:
On 10/2/25 10: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 <[email protected]>
---
Changes in v2:
- Fixup smh_flen and smh_read to allow for > 2^31 byte files
- Link to v1: 
https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=https%3a%2f%2flore.kernel.org%2fr%2f20251002%2dfs%5fsemihosting%2dv1%2d1%2d1124e6becc51%40linaro.org&umid=ee8de4df-2479-48c7-83ae-b302789e6a41&rct=1759415558&auth=d807158c60b7d2502abde8a2fc01f40662980862-944860761829276b5cf613a133926bf7544353ff
---
  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;

This is completely wrong. long is 32-bits on 32-bit systems, so you
cannot get the upper 64 bits just by casting to a size_t.

--Sean

FWIW I was not attempting to get to 64 bits in a 32 bit system. long is 31 bits plus a sign bit. All I was attempting was to get to 32 bits.

Andrew

Reply via email to