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

[Andrew Goodbody]

On 14/10/2025 17:21, Sean Anderson wrote:
> On 10/14/25 12:03, Andrew Goodbody wrote:
>> The sys calls SYS_FLEN was being used in a way that did ot 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.
>>
>> This issue was found by Smatch.
>>
>> Signed-off-by: Andrew Goodbody <andrew.goodbody at linaro.org>
>> ---
>>   common/spl/spl_semihosting.c |  6 +++---
>>   fs/semihostingfs.c           | 14 ++++++++------
>>   include/semihosting.h        |  5 +++--
>>   lib/semihosting.c            |  6 ++++--
>>   4 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/common/spl/spl_semihosting.c b/common/spl/spl_semihosting.c
>> index cff6f5351e265136b3effe483fd5d641fdd109f8..fb578a274d0d937b2ed5bd13e505bcefd4491799 100644
>> --- a/common/spl/spl_semihosting.c
>> +++ b/common/spl/spl_semihosting.c
>> @@ -28,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);
>> @@ -37,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/fs/semihostingfs.c b/fs/semihostingfs.c
>> index 15f58d578b65f7dbbadce19093546f5bdb46c0c3..6a4d0aaaf2fdaf8da154e59677839a5c7a4889e9 100644
>> --- a/fs/semihostingfs.c
>> +++ b/fs/semihostingfs.c
>> @@ -35,10 +35,10 @@ 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;
>> @@ -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 < 0)
>> +		return ret;
>>   
>>   	*result = size;
>>   	return 0;
>> diff --git a/include/semihosting.h b/include/semihosting.h
>> index 42832d86c711ff04e91b6b739c80629253f6d30a..60535199b9532ce16620aea3ca1ea741c5b73766 100644
>> --- a/include/semihosting.h
>> +++ b/include/semihosting.h
>> @@ -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 e49b16b0aa34ab04b6c8949a3e7cab539fdbef82..547a936e17f38204e840035a50f481e57d159592 100644
>> --- a/lib/semihosting.c
>> +++ b/lib/semihosting.c
>> @@ -154,7 +154,7 @@ long smh_close(long fd)
>>   	return 0;
>>   }
>>   
>> -long smh_flen(long fd)
>> +int smh_flen(long fd, size_t *size)
>>   {
>>   	long ret;
>>   
>> @@ -163,7 +163,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)
>>
> 
> Nack, for reasons outlined on v3.
> 
> This should just be
> 
>      if (ret < 0)
>          return ret == -1 ? smh_errno() : -EOVERFLOW;
> 
> --Sean

I am not going to spend any more time on this. There is a real bug that 
is addressed by V1.
https://patchwork.ozlabs.org/project/uboot/patch/20251002-fs_semihosting-v1-1-1124e6becc51@linaro.org/
Please consider applying that.

Andrew