Am 2. Oktober 2025 17:42:32 MESZ schrieb Sean Anderson <[email protected]>: >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
size_t has 32 bits on a 32 bit machine. The type of the r1 register is unsigned long. Using signed long here may be argued about. As far as GCC and LLVM are concerned: sizeof(long) == sizeof(size_t). Best regards Heinrich
