[PATCH 1/1] fs: fix smh_fs_read_at()
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) + 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;
size = smh_read(fd, buffer, maxsize); smh_close(fd);
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); smh_close(fd);
This interface is nuts, but this patch does successfully implement it...
--Sean
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.
https://developer.arm.com/documentation/dui0471/m/what-is-semihosting-/sys-r... 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
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.
--Sean
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.
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
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
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.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?
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
On 6/2/23 12:54, Heinrich Schuchardt wrote:
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.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?
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.
OK, then I agree that we should have a loop in read/write. And this should be documented :)
--Sean
participants (2)
-
Heinrich Schuchardt -
Sean Anderson