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.schucha...@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=https%3a%2f%2fdeveloper.arm.com%2fdocumentation%2fdui0471%2fm%2fwhat%2dis%2dsemihosting%2d%2fsys%2dread%2d%2d0x06%2d&umid=985b6a56-4e08-43b2-84c0-f256a70807de&auth=d807158c60b7d2502abde8a2fc01f40662980862-076ac538dd94fd766377877ad7a42d77df41830f
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.
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.
Best regards
Heinrich
--Sean
