[v1] target/arm: Fix signed integer overflow undefined behavior.

[PATCH] target/arm: Fix signed integer overflow undefined behavior.

Posted by Stephen Longfield 11 months, 3 weeks ago

The problem is internal to t32_expandimm_imm, the imm intermediate
immediate value. This value is sourced from x, which always comes from
the return of a deposit32 call, which returns uint32_t already.

It's extracted via: int imm = extract32(x, 0, 8);, so the value will be
between 0-255

It is then multiplied by one of 1, 0x00010001, 0x01000100, 0x01010101,
or 0x80.

Values between 128-255 multiplied by 0x01000100 or 0x01010101 will cause
the upper bit to get set, which is a signed integer overflow. From
Chapter 6.5, paragraph 5 of the C11 spec:
https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf this is
undefined behavior.

Though this is a minor undefined behavior, I'd like to see this fixed,
since the error is showing up when I enable clang's sanitizers while
looking for other issues.

Signed-off-by: Stephen Longfield <slongfield@google.com>
Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
---
 target/arm/tcg/translate.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 68ac393415..8770f0ce1c 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -3508,9 +3508,9 @@ static int t32_expandimm_rot(DisasContext *s, int x)
 }

 /* Return the unrotated immediate from T32ExpandImm.  */
-static int t32_expandimm_imm(DisasContext *s, int x)
+static uint32_t t32_expandimm_imm(DisasContext *s, uint32_t x)
 {
-    int imm = extract32(x, 0, 8);
+    uint32_t imm = extract32(x, 0, 8);

     switch (extract32(x, 8, 4)) {
     case 0: /* XY */
--
2.48.1.601.g30ceb7b040-goog

Re: [PATCH] target/arm: Fix signed integer overflow undefined behavior.

Posted by Peter Maydell 11 months, 3 weeks ago

On Tue, 18 Feb 2025 at 22:22, Stephen Longfield <slongfield@google.com> wrote:
>
> The problem is internal to t32_expandimm_imm, the imm intermediate
> immediate value. This value is sourced from x, which always comes from
> the return of a deposit32 call, which returns uint32_t already.
>
> It's extracted via: int imm = extract32(x, 0, 8);, so the value will be
> between 0-255
>
> It is then multiplied by one of 1, 0x00010001, 0x01000100, 0x01010101,
> or 0x80.
>
> Values between 128-255 multiplied by 0x01000100 or 0x01010101 will cause
> the upper bit to get set, which is a signed integer overflow. From
> Chapter 6.5, paragraph 5 of the C11 spec:
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf this is
> undefined behavior.

QEMU always compiles with -fwrapv. This means that this integer
overflow is not undefined behaviour in our dialect of C.

> Though this is a minor undefined behavior, I'd like to see this fixed,
> since the error is showing up when I enable clang's sanitizers while
> looking for other issues.

If clang's sanitizer reports the overflow as UB when built with
-fwrapv, that is a bug in the sanitizer and you should get
it fixed in clang.

We use and rely on 2s complement handling of signed integers
in a lot of places, so if you try to find and fix them
all you're going to be playing a pointless game of whackamole.

> Signed-off-by: Stephen Longfield <slongfield@google.com>
> Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> ---
>  target/arm/tcg/translate.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
> index 68ac393415..8770f0ce1c 100644
> --- a/target/arm/tcg/translate.c
> +++ b/target/arm/tcg/translate.c
> @@ -3508,9 +3508,9 @@ static int t32_expandimm_rot(DisasContext *s, int x)
>  }
>
>  /* Return the unrotated immediate from T32ExpandImm.  */
> -static int t32_expandimm_imm(DisasContext *s, int x)
> +static uint32_t t32_expandimm_imm(DisasContext *s, uint32_t x)

This function is following the API for decodetree !function
filters, which return 'int', not 'uint32_t'.

>  {
> -    int imm = extract32(x, 0, 8);
> +    uint32_t imm = extract32(x, 0, 8);

Given what we're doing in the function, it is reasonable
to make this a uint32_t, though.

>
>      switch (extract32(x, 8, 4)) {
>      case 0: /* XY */

thanks
-- PMM

Re: [PATCH] target/arm: Fix signed integer overflow undefined behavior.

Posted by Stephen Longfield 11 months, 3 weeks ago

On Wed, Feb 19, 2025 at 7:26 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Tue, 18 Feb 2025 at 22:22, Stephen Longfield <slongfield@google.com>
> wrote:
> >
> > The problem is internal to t32_expandimm_imm, the imm intermediate
> > immediate value. This value is sourced from x, which always comes from
> > the return of a deposit32 call, which returns uint32_t already.
> >
> > It's extracted via: int imm = extract32(x, 0, 8);, so the value will be
> > between 0-255
> >
> > It is then multiplied by one of 1, 0x00010001, 0x01000100, 0x01010101,
> > or 0x80.
> >
> > Values between 128-255 multiplied by 0x01000100 or 0x01010101 will cause
> > the upper bit to get set, which is a signed integer overflow. From
> > Chapter 6.5, paragraph 5 of the C11 spec:
> > https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf this is
> > undefined behavior.
>
> QEMU always compiles with -fwrapv. This means that this integer
> overflow is not undefined behaviour in our dialect of C.
>
> > Though this is a minor undefined behavior, I'd like to see this fixed,
> > since the error is showing up when I enable clang's sanitizers while
> > looking for other issues.
>
> If clang's sanitizer reports the overflow as UB when built with
> -fwrapv, that is a bug in the sanitizer and you should get
> it fixed in clang.
> We use and rely on 2s complement handling of signed integers
> in a lot of places, so if you try to find and fix them
> all you're going to be playing a pointless game of whackamole.
>

Yeah, I was running with `-ftrapv` instead of `-fwrapv` looking for errors
in other code.
This was the only place that got flagged, but sounds like that's likely
just an artifact
of the test I was running. (Though, there is a vanishingly small amount of
math
done on `int`s in target/arm/tcg/translate.c, which also probably helps.)


> > Signed-off-by: Stephen Longfield <slongfield@google.com>
> > Signed-off-by: Roque Arcudia Hernandez <roqueh@google.com>
> > ---
> >  target/arm/tcg/translate.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
> > index 68ac393415..8770f0ce1c 100644
> > --- a/target/arm/tcg/translate.c
> > +++ b/target/arm/tcg/translate.c
> > @@ -3508,9 +3508,9 @@ static int t32_expandimm_rot(DisasContext *s, int
> x)
> >  }
> >
> >  /* Return the unrotated immediate from T32ExpandImm.  */
> > -static int t32_expandimm_imm(DisasContext *s, int x)
> > +static uint32_t t32_expandimm_imm(DisasContext *s, uint32_t x)
>
> This function is following the API for decodetree !function
> filters, which return 'int', not 'uint32_t'.
>
> >  {
> > -    int imm = extract32(x, 0, 8);
> > +    uint32_t imm = extract32(x, 0, 8);
>
> Given what we're doing in the function, it is reasonable
> to make this a uint32_t, though.
>

Changing this to uint32_t is sufficient for me.

I'll send out a v2 of the patch.


> >
> >      switch (extract32(x, 8, 4)) {
> >      case 0: /* XY */
>
> thanks
> -- PMM
>

Thank you for your comprehensive and quick feedback!

--Stephen