On 6/2/23 12:21, Heinrich Schuchardt wrote:
On 6/2/23 17:56, Sean Anderson wrote:
On 6/2/23 04:48, Heinrich Schuchardt wrote:
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@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.
Sounds reasonable.
https://cas5-0-urlprotect.trendmicro.com:443/wis/clicktime/v1/query?url=http... 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?
We return the number of bytes read. It's up to the caller to loop if they want.
IMO for efficiency, the host should handle short reads to avoid multiple semihosting calls.
Have a look at the QEMU code. host_read() calls read() once. So if the underlying host file system does not read all bytes in the first invocation (which is allowable), SYS_READ will not do so either.
Yes, I know that some hosts do not do this, but they should for efficiency.
U-Boot's do_load() calls _fs_read() only once. It expects the semihosting filesystem to return the complete file.
So there seems to be a gap in U-Boot's semihosting code.
Isn't that what the fs_read's actread parameter is for?
--Sean