On 6/2/23 18:43, Sean Anderson wrote:
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.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.
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?
All usages of fs_read() expect that actread returns the size of the
complete file, e.g.
- sysboot_read_file()
- do_load()
- splash_load_fs()
Other U-Boot file systems like FAT, EXT4 conform to this.
Unfortunately struct fstype_info is not documented at all and the
documentation of fs_read() in include/fs.h does not point this out.
Best regards
Heinrich
