[PATCH 1/1] fs: fix smh_fs_read_at()

[Heinrich Schuchardt]

On 5/18/23 17:17, Sean Anderson wrote:
> On 5/17/23 06:23, Heinrich Schuchardt wrote:
>> The return value of smh_flen() is written to size and not to ret. But ret
>> is checked. We can avoid calling smh_flen() by setting maxsize to LONG_MAX
>> if it is not set yet.
>>
>> Check input parameters.
>>
>> Fixes: f676b45151c3 ("fs: Add semihosting filesystem")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>   fs/semihostingfs.c | 14 +++++---------
>>   1 file changed, 5 insertions(+), 9 deletions(-)
>>
>> diff --git a/fs/semihostingfs.c b/fs/semihostingfs.c
>> index 96eb3349a2..8a7d4da884 100644
>> --- a/fs/semihostingfs.c
>> +++ b/fs/semihostingfs.c
>> @@ -25,6 +25,9 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer,
>>   {
>>   	long fd, size, ret;
>>   
>> +	if (pos > LONG_MAX || maxsize > LONG_MAX)
> 
> Should be ULONG_MAX. The type should really be ulong but isn't.
> 
>> +		return -EINVAL;
>> +
>>   	fd = smh_open(filename, MODE_READ | MODE_BINARY);
>>   	if (fd < 0)
>>   		return fd;
>> @@ -33,15 +36,8 @@ static int smh_fs_read_at(const char *filename, loff_t pos, void *buffer,
>>   		smh_close(fd);
>>   		return ret;
>>   	}
>> -	if (!maxsize) {
>> -		size = smh_flen(fd);
>> -		if (ret < 0) {
>> -			smh_close(fd);
>> -			return size;
>> -		}
>> -
>> -		maxsize = size;
>> -	}
>> +	if (!maxsize)
>> +		maxsize = LONG_MAX;
> 
> Same here.
> 
>>   
>>   	size = smh_read(fd, buffer, maxsize);

Thanks for reviewing.

Just changing to ULONG_MAX will not work, because smh_read() returns 
errors as negative numbers.

We would need to change the interface of smh_read to:

int smh_read(long fd, void *memp, unsigned long *len)

Then we could return the error code and the number of bytes read separately.

https://developer.arm.com/documentation/dui0471/m/what-is-semihosting-/sys-read--0x06-
describes that SYS_READ may return less bytes than requested. This does 
not signify that EOF has been reached but may simply reflect the 
behavior of the underlying operating system (cf. 'man 2 read').

So shouldn't we loop here until all bytes are read?

Best regards

Heinrich

>>   	smh_close(fd);
> 
> This interface is nuts, but this patch does successfully implement it...
> 
> --Sean