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

Commit Message

Stephen Longfield Feb. 19, 2025, 4:55 p.m. UTC

The problem is internal to t32_expandimm_imm, the imm intermediate
immediate value.

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.

Changes from v1: From [e-mail]'s review, only changing
the internal representation from int to uint32_t, and leaving the API
types the same.

Signed-off-by: Stephen Longfield <[e-mail]>
Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
---
 target/arm/tcg/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.48.1.601.g30ceb7b040-goog

Comments

Peter Maydell Feb. 20, 2025, 2:15 p.m. UTC | #1

On Wed, 19 Feb 2025 at 16:55, Stephen Longfield <[e-mail]> wrote:
>
> The problem is internal to t32_expandimm_imm, the imm intermediate
> immediate value.
>
> 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.
>
> Changes from v1: From [e-mail]'s review, only changing
> the internal representation from int to uint32_t, and leaving the API
> types the same.
>
> Signed-off-by: Stephen Longfield <[e-mail]>
> Signed-off-by: Roque Arcudia Hernandez <[e-mail]>
> ---
>  target/arm/tcg/translate.c | 2 +-

Thanks; I've applied this to target-arm.next, with a rewritten
commit message:

    target/arm: Use uint32_t in t32_expandimm_imm()

    In t32_expandimm_imm(), we take an 8 bit value XY and construct a
    32-bit value which might be of the form XY, 00XY00XY, XY00XY00, or
    XYXYXYXY.  We do this with multiplications, and we use an 'int' type.
    For the cases where we're setting the high byte of the 32-bit value
    to XY, this means that we do an integer multiplication that might
    overflow, and rely on the -fwrapv semantics to keep this from being
    undefined behaviour.

    It's clearer to use an unsigned type here, because we're really
    doing operations on the value considered as a set of bits. The
    result is the same.

    The return value from the function remains 'int', because this
    is a decodetree !function function, and follows the API for those
    functions.

-- PMM

Patch

diff --git a/target/arm/tcg/translate.c b/target/arm/tcg/translate.c
index 68ac393415..d8225b77c8 100644
--- a/target/arm/tcg/translate.c
+++ b/target/arm/tcg/translate.c
@@ -3510,7 +3510,7 @@  static int t32_expandimm_rot(DisasContext *s, int x)
 /* Return the unrotated immediate from T32ExpandImm.  */
 static int t32_expandimm_imm(DisasContext *s, int x)
 {
-    int imm = extract32(x, 0, 8);
+    uint32_t imm = extract32(x, 0, 8);

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