[PATCH 0/3] semihosting: Fix a few semihosting bugs

[PATCH 0/3] semihosting: Fix a few semihosting bugs

[Sean Anderson]

While discussing [1], it came to my attention that QEMU does not
properly truncate/error SYS_FLEN on 32-bit systems. Fix this, and some
other bugs with GDB File I/O that I found while working on this series.
That said, GDB File I/O has been substantially broken for two years now,
so it makes me wonder if anyone actually uses it! It would certainly
simplify the implementation if we didn't have to support it.

[1] https://lore.kernel.org/u-boot/20251017195322.GF6688@bill-the-cat/T/#m493c42570d3103b8c606c5f50faeb78d27719de6


Sean Anderson (3):
  gdbstub: Fix %s formatting
  semihosting: Fix GDB File-I/O FLEN
  semihosting: Check for overflow in FLEN on 32-bit systems

 gdbstub/syscalls.c            |  2 +-
 semihosting/arm-compat-semi.c | 22 +++++++++++++++-------
 2 files changed, 16 insertions(+), 8 deletions(-)

-- 
2.35.1.1320.gc452695387.dirty

[PATCH 1/3] gdbstub: Fix %s formatting

[Sean Anderson]

The format string for %s has two format characters. This causes it to
emit strings like "466f5bd8/6x" instead of "466f5bd8/6". GDB detects
this and returns EIO, causing all open File I/O calls to fail.

Fixes: 0820a075af ("gdbstub: Adjust gdb_do_syscall to only use uint32_t and uint64_t")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 gdbstub/syscalls.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c
index e855df21ab..d8bb90cc1c 100644
--- a/gdbstub/syscalls.c
+++ b/gdbstub/syscalls.c
@@ -127,7 +127,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
             case 's':
                 i64 = va_arg(va, uint64_t);
                 i32 = va_arg(va, uint32_t);
-                p += snprintf(p, p_end - p, "%" PRIx64 "/%x" PRIx32, i64, i32);
+                p += snprintf(p, p_end - p, "%" PRIx64 "/%" PRIx32, i64, i32);
                 break;
             default:
             bad_format:
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 1/3] gdbstub: Fix %s formatting

[Richard Henderson]

On 10/17/25 14:35, Sean Anderson wrote:
> The format string for %s has two format characters. This causes it to
> emit strings like "466f5bd8/6x" instead of "466f5bd8/6". GDB detects
> this and returns EIO, causing all open File I/O calls to fail.
> 
> Fixes: 0820a075af ("gdbstub: Adjust gdb_do_syscall to only use uint32_t and uint64_t")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>   gdbstub/syscalls.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdbstub/syscalls.c b/gdbstub/syscalls.c
> index e855df21ab..d8bb90cc1c 100644
> --- a/gdbstub/syscalls.c
> +++ b/gdbstub/syscalls.c
> @@ -127,7 +127,7 @@ void gdb_do_syscall(gdb_syscall_complete_cb cb, const char *fmt, ...)
>               case 's':
>                   i64 = va_arg(va, uint64_t);
>                   i32 = va_arg(va, uint32_t);
> -                p += snprintf(p, p_end - p, "%" PRIx64 "/%x" PRIx32, i64, i32);
> +                p += snprintf(p, p_end - p, "%" PRIx64 "/%" PRIx32, i64, i32);
>                   break;
>               default:
>               bad_format:

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH 1/3] gdbstub: Fix %s formatting

[Alex Bennée]

Sean Anderson <sean.anderson@linux.dev> writes:

> The format string for %s has two format characters. This causes it to
> emit strings like "466f5bd8/6x" instead of "466f5bd8/6". GDB detects
> this and returns EIO, causing all open File I/O calls to fail.
>
> Fixes: 0820a075af ("gdbstub: Adjust gdb_do_syscall to only use uint32_t and uint64_t")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 2/3] semihosting: Fix GDB File-I/O FLEN

[Sean Anderson]

fstat returns 0 on success and -1 on error. Since we have already
checked for error, ret must be zero. Therefore, any call to fstat on a
non-empty file will return -1/EOVERFLOW.

Restore the original logic that just did a byteswap. I don't really know
what the intention of the fixed commit was.

Fixes: a6300ed6b7 ("semihosting: Split out semihost_sys_flen")
Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 semihosting/arm-compat-semi.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index 6100126796..c5a07cb947 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -316,10 +316,7 @@ common_semi_flen_fstat_cb(CPUState *cs, uint64_t ret, int err)
                                 &size, 8, 0)) {
             ret = -1, err = EFAULT;
         } else {
-            size = be64_to_cpu(size);
-            if (ret != size) {
-                ret = -1, err = EOVERFLOW;
-            }
+            ret = be64_to_cpu(size);
         }
     }
     common_semi_cb(cs, ret, err);
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 2/3] semihosting: Fix GDB File-I/O FLEN

[Alex Bennée]

Sean Anderson <sean.anderson@linux.dev> writes:

> fstat returns 0 on success and -1 on error. Since we have already
> checked for error, ret must be zero. Therefore, any call to fstat on a
> non-empty file will return -1/EOVERFLOW.
>
> Restore the original logic that just did a byteswap. I don't really know
> what the intention of the fixed commit was.
>
> Fixes: a6300ed6b7 ("semihosting: Split out semihost_sys_flen")
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
>
>  semihosting/arm-compat-semi.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index 6100126796..c5a07cb947 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -316,10 +316,7 @@ common_semi_flen_fstat_cb(CPUState *cs, uint64_t ret, int err)
>                                  &size, 8, 0)) {
>              ret = -1, err = EFAULT;
>          } else {
> -            size = be64_to_cpu(size);
> -            if (ret != size) {
> -                ret = -1, err = EOVERFLOW;
> -            }
> +            ret = be64_to_cpu(size);
>          }
>      }
>      common_semi_cb(cs, ret, err);

well this sent me on a rat hole to figure out the be64_to_cpu. Really I
think the callback function should be renamed to gdb_semi_flen_fstat_cb
because it is only called for the GDB File I/O implementation.

Otherwise the logic looks fine:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

[PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Sean Anderson]

When semihosting 32-bit systems, the return value of FLEN will be stored
in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.
This matches the behavior of stat(2). Static files don't need to be
checked, since are always small.

Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
---

 semihosting/arm-compat-semi.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
index c5a07cb947..57453ca6be 100644
--- a/semihosting/arm-compat-semi.c
+++ b/semihosting/arm-compat-semi.c
@@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
     return sp - 64;
 }
 
+static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
+{
+    CPUArchState *env = cpu_env(cs);
+
+    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {
+            ret = -1, err = EOVERFLOW;
+    }
+    common_semi_cb(cs, ret, err);
+}
+
+
 static void
-common_semi_flen_fstat_cb(CPUState *cs, uint64_t ret, int err)
+common_semi_flen_gdb_cb(CPUState *cs, uint64_t ret, int err)
 {
     if (!err) {
         /* The size is always stored in big-endian order, extract the value. */
@@ -319,7 +330,7 @@ common_semi_flen_fstat_cb(CPUState *cs, uint64_t ret, int err)
             ret = be64_to_cpu(size);
         }
     }
-    common_semi_cb(cs, ret, err);
+    common_semi_flen_cb(cs, ret, err);
 }
 
 static void
@@ -517,7 +528,7 @@ void do_common_semihosting(CPUState *cs)
 
     case TARGET_SYS_FLEN:
         GET_ARG(0);
-        semihost_sys_flen(cs, common_semi_flen_fstat_cb, common_semi_cb,
+        semihost_sys_flen(cs, common_semi_flen_gdb_cb, common_semi_flen_cb,
                           arg0, common_semi_flen_buf(cs));
         break;
 
-- 
2.35.1.1320.gc452695387.dirty

Re: [PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Heinrich Schuchardt]

On 10/17/25 23:35, Sean Anderson wrote:
> When semihosting 32-bit systems, the return value of FLEN will be stored
> in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.
> This matches the behavior of stat(2). Static files don't need to be
> checked, since are always small.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>   semihosting/arm-compat-semi.c | 17 ++++++++++++++---
>   1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> index c5a07cb947..57453ca6be 100644
> --- a/semihosting/arm-compat-semi.c
> +++ b/semihosting/arm-compat-semi.c
> @@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
>       return sp - 64;
>   }
>   
> +static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
> +{
> +    CPUArchState *env = cpu_env(cs);
> +
> +    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {


The issue with the current implementation is that files with file sizes 
over 4 GiB will be reported as file size < 4 -GiB on 32bit systems. 
Thanks for addressing this.

But unfortunately with your change you are additionally dropping support 
for file sizes 2 GiB to 4 GiB on 32bit devices. This should be avoided.

The semihosting specification specifies that the value returned in r0 
should be -1 if an error occurs. So on 32 bit systems 0xffffffff should 
be returned.

As file sizes cannot be negative there is not reason to assume that the 
value in r0 has to be interpreted by semihosting clients as signed.

Please, change your commit to check against 0xffffffff.

It might make sense to contact ARM to make their specification clearer.

Best regards

Heinrich


> +            ret = -1, err = EOVERFLOW;
> +    }
> +    common_semi_cb(cs, ret, err);
> +}
> +
> +
>   static void
> -common_semi_flen_fstat_cb(CPUState *cs, uint64_t ret, int err)
> +common_semi_flen_gdb_cb(CPUState *cs, uint64_t ret, int err)
>   {
>       if (!err) {
>           /* The size is always stored in big-endian order, extract the value. */
> @@ -319,7 +330,7 @@ common_semi_flen_fstat_cb(CPUState *cs, uint64_t ret, int err)
>               ret = be64_to_cpu(size);
>           }
>       }
> -    common_semi_cb(cs, ret, err);
> +    common_semi_flen_cb(cs, ret, err);
>   }
>   
>   static void
> @@ -517,7 +528,7 @@ void do_common_semihosting(CPUState *cs)
>   
>       case TARGET_SYS_FLEN:
>           GET_ARG(0);
> -        semihost_sys_flen(cs, common_semi_flen_fstat_cb, common_semi_cb,
> +        semihost_sys_flen(cs, common_semi_flen_gdb_cb, common_semi_flen_cb,
>                             arg0, common_semi_flen_buf(cs));
>           break;
>   

Re: [PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Sean Anderson]

On 10/18/25 03:21, Heinrich Schuchardt wrote:
> On 10/17/25 23:35, Sean Anderson wrote:
>> When semihosting 32-bit systems, the return value of FLEN will be stored
>> in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.
>> This matches the behavior of stat(2). Static files don't need to be
>> checked, since are always small.
>>
>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> ---
>>
>>   semihosting/arm-compat-semi.c | 17 ++++++++++++++---
>>   1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
>> index c5a07cb947..57453ca6be 100644
>> --- a/semihosting/arm-compat-semi.c
>> +++ b/semihosting/arm-compat-semi.c
>> @@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
>>       return sp - 64;
>>   }
>>   +static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
>> +{
>> +    CPUArchState *env = cpu_env(cs);
>> +
>> +    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {
> 
> 
> The issue with the current implementation is that files with file sizes over 4 GiB will be reported as file size < 4 -GiB on 32bit systems. Thanks for addressing this.
> 
> But unfortunately with your change you are additionally dropping support for file sizes 2 GiB to 4 GiB on 32bit devices. This should be avoided.
> 
> The semihosting specification specifies that the value returned in r0 should be -1 if an error occurs. So on 32 bit systems 0xffffffff should be returned.
> 
> As file sizes cannot be negative there is not reason to assume that the value in r0 has to be interpreted by semihosting clients as signed.
> 
> Please, change your commit to check against 0xffffffff.
> 
> It might make sense to contact ARM to make their specification clearer.

stat(2) will return -1/EOVERFLOW on 32-bit hosts for files over 2 GiB. I believe we should be consistent.

--Sean

Re: [PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Heinrich Schuchardt]

[-- Attachment #1: Type: text/plain, Size: 2249 bytes --]

Sean Anderson <sean.anderson@linux.dev> schrieb am Mo., 20. Okt. 2025,
16:21:

> On 10/18/25 03:21, Heinrich Schuchardt wrote:
> > On 10/17/25 23:35, Sean Anderson wrote:
> >> When semihosting 32-bit systems, the return value of FLEN will be stored
> >> in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.

>> This matches the behavior of stat(2). Static files don't need to be
> >> checked, since are always small.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >>
> >>   semihosting/arm-compat-semi.c | 17 ++++++++++++++---
> >>   1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/semihosting/arm-compat-semi.c
> b/semihosting/arm-compat-semi.c
> >> index c5a07cb947..57453ca6be 100644
> >> --- a/semihosting/arm-compat-semi.c
> >> +++ b/semihosting/arm-compat-semi.c
> >> @@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
> >>       return sp - 64;
> >>   }
> >>   +static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
> >> +{
> >> +    CPUArchState *env = cpu_env(cs);
> >> +
> >> +    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {
> >
> >
> > The issue with the current implementation is that files with file sizes
> over 4 GiB will be reported as file size < 4 -GiB on 32bit systems. Thanks
> for addressing this.
> >
> > But unfortunately with your change you are additionally dropping support
> for file sizes 2 GiB to 4 GiB on 32bit devices. This should be avoided.
> >
> > The semihosting specification specifies that the value returned in r0
> should be -1 if an error occurs. So on 32 bit systems 0xffffffff should be
> returned.
> >
> > As file sizes cannot be negative there is not reason to assume that the
> value in r0 has to be interpreted by semihosting clients as signed.
> >
> > Please, change your commit to check against 0xffffffff.
> >
> > It might make sense to contact ARM to make their specification clearer.
>
> stat(2) will return -1/EOVERFLOW on 32-bit hosts for files over 2 GiB. I
> believe we should be consistent.
>

That may have been true historically.

Current 32-bit Linux supports 64-bit file systems and reports the length of
files beyond 2 GiB without error.

Best regards

Heinrich

[-- Attachment #2: Type: text/html, Size: 3337 bytes --]

Re: [PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Sean Anderson]

On 10/20/25 11:33, Heinrich Schuchardt wrote:
> 
> 
> Sean Anderson <sean.anderson@linux.dev <mailto:sean.anderson@linux.dev>> schrieb am Mo., 20. Okt. 2025, 16:21:
> 
>     On 10/18/25 03:21, Heinrich Schuchardt wrote:
>     > On 10/17/25 23:35, Sean Anderson wrote:
>     >> When semihosting 32-bit systems, the return value of FLEN will be stored
>     >> in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.
> 
>     >> This matches the behavior of stat(2). Static files don't need to be
>     >> checked, since are always small.
>     >>
>     >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev <mailto:sean.anderson@linux.dev>>
>     >> ---
>     >>
>     >>   semihosting/arm-compat-semi.c | 17 ++++++++++++++---
>     >>   1 file changed, 14 insertions(+), 3 deletions(-)
>     >>
>     >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
>     >> index c5a07cb947..57453ca6be 100644
>     >> --- a/semihosting/arm-compat-semi.c
>     >> +++ b/semihosting/arm-compat-semi.c
>     >> @@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
>     >>       return sp - 64;
>     >>   }
>     >>   +static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
>     >> +{
>     >> +    CPUArchState *env = cpu_env(cs);
>     >> +
>     >> +    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {
>     >
>     >
>     > The issue with the current implementation is that files with file sizes over 4 GiB will be reported as file size < 4 -GiB on 32bit systems. Thanks for addressing this.
>     >
>     > But unfortunately with your change you are additionally dropping support for file sizes 2 GiB to 4 GiB on 32bit devices. This should be avoided.
>     >
>     > The semihosting specification specifies that the value returned in r0 should be -1 if an error occurs. So on 32 bit systems 0xffffffff should be returned.
>     >
>     > As file sizes cannot be negative there is not reason to assume that the value in r0 has to be interpreted by semihosting clients as signed.
>     >
>     > Please, change your commit to check against 0xffffffff.
>     >
>     > It might make sense to contact ARM to make their specification clearer.
> 
>     stat(2) will return -1/EOVERFLOW on 32-bit hosts for files over 2 GiB. I believe we should be consistent.
> 
> 
> That may have been true historically.
> 
> Current 32-bit Linux supports 64-bit file systems and reports the length of files beyond 2 GiB without error.

Yes, but 32-bit semihosting targets only support 32-bit file lengths. So
I believe we should behave the same way as if the host had a 32-bit
off_t.

And as I've mentioned elsewhere, I think that virtio is a much better
way to transfer large files.

--Sean

Re: [PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Peter Maydell]

On Mon, 20 Oct 2025 at 16:01, Sean Anderson <sean.anderson@linux.dev> wrote:
>
> On 10/18/25 03:21, Heinrich Schuchardt wrote:
> > On 10/17/25 23:35, Sean Anderson wrote:
> >> When semihosting 32-bit systems, the return value of FLEN will be stored
> >> in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.
> >> This matches the behavior of stat(2). Static files don't need to be
> >> checked, since are always small.
> >>
> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> >> ---
> >>
> >>   semihosting/arm-compat-semi.c | 17 ++++++++++++++---
> >>   1 file changed, 14 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
> >> index c5a07cb947..57453ca6be 100644
> >> --- a/semihosting/arm-compat-semi.c
> >> +++ b/semihosting/arm-compat-semi.c
> >> @@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
> >>       return sp - 64;
> >>   }
> >>   +static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
> >> +{
> >> +    CPUArchState *env = cpu_env(cs);
> >> +
> >> +    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {
> >
> >
> > The issue with the current implementation is that files with file sizes over 4 GiB will be reported as file size < 4 -GiB on 32bit systems. Thanks for addressing this.
> >
> > But unfortunately with your change you are additionally dropping support for file sizes 2 GiB to 4 GiB on 32bit devices. This should be avoided.
> >
> > The semihosting specification specifies that the value returned in r0 should be -1 if an error occurs. So on 32 bit systems 0xffffffff should be returned.
> >
> > As file sizes cannot be negative there is not reason to assume that the value in r0 has to be interpreted by semihosting clients as signed.
> >
> > Please, change your commit to check against 0xffffffff.
> >
> > It might make sense to contact ARM to make their specification clearer.
>
> stat(2) will return -1/EOVERFLOW on 32-bit hosts for files over 2 GiB. I believe we should be consistent.

I can see both sides of this one -- the semihosting spec
is pretty old and was designed (to the extent it was designed)
back in an era when 2GB of RAM or a 2GB file was pretty
implausible sounding. (And today there's not much appetite
for making updates to it for AArch32, because 32-bit is
the past, not the future, and in any case you have to deal
with all the existing implementations of the spec so
changes are super painful to try to promulgate.)

Our QEMU implementation of SYS_ISERROR() says "anything that's
a negative 32-bit integer is an error number" for 32-bit hosts,
which I suppose you might count on the side of "check
against INT32_MAX".

I think overall that if we think that anybody is or might be
using semihosting with files in that 2..4GB size, we should
err on the side of preserving that functionality. Otherwise
somebody will report it as a bug and we'll want to fix it
as a regression. And it doesn't seem impossible that somebody
out there is doing so.

If we report 2..4GB file sizes as if the file size value
was an unsigned integer, then clients that expect that will
work, and clients that treat any negative 32-bit value as
an error will also work in the sense that they'll handle it
as an error in the same way they would have done for -1.
So that is the safest approach from a back-compat point
of view, and I think that's what I lean towards doing.

thanks
-- PMM

Re: [PATCH 3/3] semihosting: Check for overflow in FLEN on 32-bit systems

[Sean Anderson]

On 10/20/25 12:33, Peter Maydell wrote:
> On Mon, 20 Oct 2025 at 16:01, Sean Anderson <sean.anderson@linux.dev> wrote:
>>
>> On 10/18/25 03:21, Heinrich Schuchardt wrote:
>> > On 10/17/25 23:35, Sean Anderson wrote:
>> >> When semihosting 32-bit systems, the return value of FLEN will be stored
>> >> in a 32-bit integer. To prevent wraparound, return -1 and set EOVERFLOW.
>> >> This matches the behavior of stat(2). Static files don't need to be
>> >> checked, since are always small.
>> >>
>> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
>> >> ---
>> >>
>> >>   semihosting/arm-compat-semi.c | 17 ++++++++++++++---
>> >>   1 file changed, 14 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/semihosting/arm-compat-semi.c b/semihosting/arm-compat-semi.c
>> >> index c5a07cb947..57453ca6be 100644
>> >> --- a/semihosting/arm-compat-semi.c
>> >> +++ b/semihosting/arm-compat-semi.c
>> >> @@ -305,8 +305,19 @@ static uint64_t common_semi_flen_buf(CPUState *cs)
>> >>       return sp - 64;
>> >>   }
>> >>   +static void common_semi_flen_cb(CPUState *cs, uint64_t ret, int err)
>> >> +{
>> >> +    CPUArchState *env = cpu_env(cs);
>> >> +
>> >> +    if (!err && !is_64bit_semihosting(env) && ret > INT32_MAX) {
>> >
>> >
>> > The issue with the current implementation is that files with file sizes over 4 GiB will be reported as file size < 4 -GiB on 32bit systems. Thanks for addressing this.
>> >
>> > But unfortunately with your change you are additionally dropping support for file sizes 2 GiB to 4 GiB on 32bit devices. This should be avoided.
>> >
>> > The semihosting specification specifies that the value returned in r0 should be -1 if an error occurs. So on 32 bit systems 0xffffffff should be returned.
>> >
>> > As file sizes cannot be negative there is not reason to assume that the value in r0 has to be interpreted by semihosting clients as signed.
>> >
>> > Please, change your commit to check against 0xffffffff.
>> >
>> > It might make sense to contact ARM to make their specification clearer.
>>
>> stat(2) will return -1/EOVERFLOW on 32-bit hosts for files over 2 GiB. I believe we should be consistent.
> 
> I can see both sides of this one -- the semihosting spec
> is pretty old and was designed (to the extent it was designed)
> back in an era when 2GB of RAM or a 2GB file was pretty
> implausible sounding. (And today there's not much appetite
> for making updates to it for AArch32, because 32-bit is
> the past, not the future, and in any case you have to deal
> with all the existing implementations of the spec so
> changes are super painful to try to promulgate.)
> 
> Our QEMU implementation of SYS_ISERROR() says "anything that's
> a negative 32-bit integer is an error number" for 32-bit hosts,
> which I suppose you might count on the side of "check
> against INT32_MAX".
> 
> I think overall that if we think that anybody is or might be
> using semihosting with files in that 2..4GB size, we should
> err on the side of preserving that functionality. Otherwise
> somebody will report it as a bug and we'll want to fix it
> as a regression. And it doesn't seem impossible that somebody
> out there is doing so.
> 
> If we report 2..4GB file sizes as if the file size value
> was an unsigned integer, then clients that expect that will
> work, and clients that treat any negative 32-bit value as
> an error will also work in the sense that they'll handle it
> as an error in the same way they would have done for -1.
> So that is the safest approach from a back-compat point
> of view, and I think that's what I lean towards doing.

Actually, some existing targets don't handle "negative" file sizes
properly. In particular, newlib generally treats the result as an int or
an off_t, which is a long (except on cygwin). Both of those types are
32 bits or smaller on (I)LP32. So if you return a 3 GiB size it will be
treated as -1 GiB. This will break lseek with SEEK_END, since newlib
uses the signed result of flen to recompute a new absolute offset.

IMO the spec is unclear, and this is reflected in the varying semantics
of differing implementations. I think returning any negative number
other than -1 is just inviting bugs.

--Sean

Re: [PATCH 0/3] semihosting: Fix a few semihosting bugs

[Alex Bennée]

Sean Anderson <sean.anderson@linux.dev> writes:

> While discussing [1], it came to my attention that QEMU does not
> properly truncate/error SYS_FLEN on 32-bit systems.

TIL that semihostingfs was a thing!

> Fix this, and some
> other bugs with GDB File I/O that I found while working on this series.
> That said, GDB File I/O has been substantially broken for two years now,
> so it makes me wonder if anyone actually uses it!

I suspect this is at the upper end of things to use semihosting for as
its real purpose is to help bootstrap things on the barest of metal
until you have enough bits going to selfhost. In QEMU land it is a
convenient way to do host calls for test cases.

We don't have much actual testing of semihosting in the tree although I
do run Peter's semihosting tests from time to time:

  https://git.linaro.org/people/peter.maydell/semihosting-tests.git/

the tests do include flen() but obviously don't cover the extreme
filesize cases or overflow.

> It would certainly
> simplify the implementation if we didn't have to support it.

While semihosting does have the concept of optional extensions SYS_FLEN
is not one of them.

>
> [1] https://lore.kernel.org/u-boot/20251017195322.GF6688@bill-the-cat/T/#m493c42570d3103b8c606c5f50faeb78d27719de6
>
>
> Sean Anderson (3):
>   gdbstub: Fix %s formatting
>   semihosting: Fix GDB File-I/O FLEN
>   semihosting: Check for overflow in FLEN on 32-bit systems
>
>  gdbstub/syscalls.c            |  2 +-
>  semihosting/arm-compat-semi.c | 22 +++++++++++++++-------
>  2 files changed, 16 insertions(+), 8 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [PATCH 0/3] semihosting: Fix a few semihosting bugs

[Sean Anderson]

On 10/20/25 11:03, Alex Bennée wrote:
> Sean Anderson <sean.anderson@linux.dev> writes:
> 
>> While discussing [1], it came to my attention that QEMU does not
>> properly truncate/error SYS_FLEN on 32-bit systems.
> 
> TIL that semihostingfs was a thing!
> 
>> Fix this, and some
>> other bugs with GDB File I/O that I found while working on this series.
>> That said, GDB File I/O has been substantially broken for two years now,
>> so it makes me wonder if anyone actually uses it!
> 
> I suspect this is at the upper end of things to use semihosting for as
> its real purpose is to help bootstrap things on the barest of metal
> until you have enough bits going to selfhost. In QEMU land it is a
> convenient way to do host calls for test cases.
> 
> We don't have much actual testing of semihosting in the tree although I
> do run Peter's semihosting tests from time to time:
> 
>   https://git.linaro.org/people/peter.maydell/semihosting-tests.git/
> 
> the tests do include flen() but obviously don't cover the extreme
> filesize cases or overflow.
> 
>> It would certainly
>> simplify the implementation if we didn't have to support it.
> 
> While semihosting does have the concept of optional extensions SYS_FLEN
> is not one of them.

The above comments refer to GDB File I/O, which currently cannot open
files due to the problem fixed in patch 1/3. FLEN is not really broken
for most use-cases (as long as the user doesn't access large files).

--Sean

Re: [PATCH 0/3] semihosting: Fix a few semihosting bugs

[Alex Bennée]

Sean Anderson <sean.anderson@linux.dev> writes:

> While discussing [1], it came to my attention that QEMU does not
> properly truncate/error SYS_FLEN on 32-bit systems. Fix this, and some
> other bugs with GDB File I/O that I found while working on this series.
> That said, GDB File I/O has been substantially broken for two years now,
> so it makes me wonder if anyone actually uses it! It would certainly
> simplify the implementation if we didn't have to support it.
>
> [1] https://lore.kernel.org/u-boot/20251017195322.GF6688@bill-the-cat/T/#m493c42570d3103b8c606c5f50faeb78d27719de6
>

Queued 1-2 to maintainer/8.2-softfreeze, thanks.

As there is some discussion on patch 3 I'll leave that for now. We can
still merge bug fixes after softfreeze but it doesn't seem like its a
critical problem.
>
> Sean Anderson (3):
>   gdbstub: Fix %s formatting
>   semihosting: Fix GDB File-I/O FLEN
>   semihosting: Check for overflow in FLEN on 32-bit systems
>
>  gdbstub/syscalls.c            |  2 +-
>  semihosting/arm-compat-semi.c | 22 +++++++++++++++-------
>  2 files changed, 16 insertions(+), 8 deletions(-)

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro