[Bug gdb/26835] New: undefined behavior in arm_analyze_prologue: shift exponent 32 is too large for 32-bit type 'unsigned int'

[Bug gdb/26835] New: undefined behavior in arm_analyze_prologue: shift exponent 32 is too large for 32-bit type 'unsigned int'

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26835

            Bug ID: 26835
           Summary: undefined behavior in arm_analyze_prologue: shift
                    exponent 32 is too large for 32-bit type 'unsigned
                    int'
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: gdb
          Assignee: unassigned at sourceware dot org
          Reporter: simark at simark dot ca
  Target Milestone: ---

See instructions to reproduce here:

https://sourceware.org/bugzilla/show_bug.cgi?id=26828#c6


I have a patch that fixes it (though it needs to be tested a bit more
thoroughly), but it lacks a test.  Ideally, we should have the equivalent of
aarch64_analyze_prologue_test, but for ARM.  That requires a bit of refactoring
of the classes that abstract memory reading in arm-tdep.c, and I don't have
time for that right now.  So I'm putting the patch here in case somebody wants
to pick it up.


>From 48596638d1998a3abcde5a4b7f5369032f9ea08b Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Tue, 3 Nov 2020 12:13:36 -0500
Subject: [PATCH] gdb/arm: avoid undefined behavior shift when decoding
 immediate value

Change-Id: Ieb1c1799bd66f8c7421384f44f5c2777b578ff8d
---
 gdb/arm-tdep.c | 39 +++++++++++++++++++++++++++------------
 1 file changed, 27 insertions(+), 12 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 82e8ec4df49c..ab286eb41a57 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -1485,6 +1485,25 @@ arm_instruction_restores_sp (unsigned int insn)
   return 0;
 }

+/* Implement immediate value decoding, as described in section A5.2.4
+   (Modified immediate constants in ARM instructions) of the ARM Architecture
+   Reference Manual.  */
+
+static uint32_t
+arm_expand_immediate (uint32_t imm)
+{
+  gdb_assert ((imm & 0xfffff000) == 0);
+
+  uint32_t unrotated_value = imm & 0xff;
+  uint32_t rotate_amount = (imm & 0xf00) >> 7;
+
+  if (rotate_amount == 0)
+    return unrotated_value;
+
+  return ((unrotated_value >> rotate_amount)
+         | (unrotated_value << (32 - rotate_amount)));
+}
+
 /* Analyze an ARM mode prologue starting at PROLOGUE_START and
    continuing no further than PROLOGUE_END.  If CACHE is non-NULL,
    fill it in.  Return the first address not recognized as a prologue
@@ -1535,20 +1554,18 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
       else if ((insn & 0xfff00000) == 0xe2800000       /* add Rd, Rn, #n */
               && pv_is_register (regs[bits (insn, 16, 19)], ARM_SP_REGNUM))
        {
-         unsigned imm = insn & 0xff;                   /* immediate value */
-         unsigned rot = (insn & 0xf00) >> 7;           /* rotate amount */
+         unsigned imm = insn & 0xfff;                   /* immediate value */
          int rd = bits (insn, 12, 15);
-         imm = (imm >> rot) | (imm << (32 - rot));
+         imm = arm_expand_immediate (imm);
          regs[rd] = pv_add_constant (regs[bits (insn, 16, 19)], imm);
          continue;
        }
       else if ((insn & 0xfff00000) == 0xe2400000       /* sub Rd, Rn, #n */
               && pv_is_register (regs[bits (insn, 16, 19)], ARM_SP_REGNUM))
        {
-         unsigned imm = insn & 0xff;                   /* immediate value */
-         unsigned rot = (insn & 0xf00) >> 7;           /* rotate amount */
+         unsigned imm = insn & 0xfff;                   /* immediate value */
          int rd = bits (insn, 12, 15);
-         imm = (imm >> rot) | (imm << (32 - rot));
+         imm = arm_expand_immediate (imm);
          regs[rd] = pv_add_constant (regs[bits (insn, 16, 19)], -imm);
          continue;
        }
@@ -1604,16 +1621,14 @@ arm_analyze_prologue (struct gdbarch *gdbarch,
        }
       else if ((insn & 0xfffff000) == 0xe24cb000)      /* sub fp, ip #n */
        {
-         unsigned imm = insn & 0xff;                   /* immediate value */
-         unsigned rot = (insn & 0xf00) >> 7;           /* rotate amount */
-         imm = (imm >> rot) | (imm << (32 - rot));
+         unsigned imm = insn & 0xfff;                  /* immediate value */
+         imm = arm_expand_immediate (imm);
          regs[ARM_FP_REGNUM] = pv_add_constant (regs[ARM_IP_REGNUM], -imm);
        }
       else if ((insn & 0xfffff000) == 0xe24dd000)      /* sub sp, sp #n */
        {
-         unsigned imm = insn & 0xff;                   /* immediate value */
-         unsigned rot = (insn & 0xf00) >> 7;           /* rotate amount */
-         imm = (imm >> rot) | (imm << (32 - rot));
+         unsigned imm = insn & 0xfff;                  /* immediate value */
+         imm = arm_expand_immediate (imm);
          regs[ARM_SP_REGNUM] = pv_add_constant (regs[ARM_SP_REGNUM], -imm);
        }
       else if ((insn & 0xffff7fff) == 0xed6d0103       /* stfe f?,
-- 
2.29.1

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26835] undefined behavior in arm_analyze_prologue: shift exponent 32 is too large for 32-bit type 'unsigned int'

[cvs-commit at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26835

--- Comment #1 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Simon Marchi <simark@sourceware.org>:

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=9ecab40c77fd414fe408967d0f92f00494aa11b9

commit 9ecab40c77fd414fe408967d0f92f00494aa11b9
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Fri Nov 13 11:58:37 2020 -0500

    gdb/arm: avoid undefined behavior shift when decoding immediate value

    When loading the code file provided in PR 26828 and GDB is build with
    UBSan, we get:

        Core was generated by `./Foo'.
        Program terminated with signal SIGABRT, Aborted.
        #0  0xb6c3809c in pthread_cond_wait () from
/home/simark/build/binutils-gdb/gdb/repo/lib/libpthread.so.0
        [Current thread is 1 (LWP 29367)]
        (gdb) bt
        /home/simark/src/binutils-gdb/gdb/arm-tdep.c:1551:30: runtime error:
shift exponent 32 is too large for 32-bit type 'unsigned int'

    The sequence of instructions at pthread_cond_wait, in the
    libpthread.so.0 library, contains this instruction with an immediate
    constant with a "rotate amount" of 0:

        e24dd044        sub     sp, sp, #68     ; 0x44

    Since arm_analyze_prologue shifts by "32 - rotate amount", it does a 32
    bit shift of a 32 bit type, which is caught by UBSan.

    Fix it by factoring out the decoding of immediates in a new function,
    arm_expand_immediate.

    I added a selftest for arm_analyze_prologue that replicates the
    instruction sequence.  Without the fix, it crashes GDB if it is build
    with --enable-ubsan.

    I initially wanted to re-use the abstract_memory_reader class already in
    arm-tdep.c, used to make arm_process_record testable.  However,
    arm_process_record and arm_analyze_prologue don't use the same kind of
    memory reading functions.  arm_process_record uses a function that
    returns an error status on failure while arm_analyze_prologue uses one
    that throws an exception.  Since i didn't want to introduce any other
    behavior change, I decided to just introduce a separate interface
    (arm_instruction_reader).  It is derived from
    abstract_instruction_reader in aarch64-tdep.c.

    gdb/ChangeLog:

            PR gdb/26835
            * arm-tdep.c (class arm_instruction_reader): New.
            (target_arm_instruction_reader): New.
            (arm_analyze_prologue): Add instruction reader parameter and use
            it.  Use arm_expand_immediate.
            (class target_arm_instruction_reader): Adjust.
            (arm_skip_prologue): Adjust.
            (arm_expand_immediate): New.
            (arm_scan_prologue): Adjust.
            (arm_analyze_prologue_test): New.
            (class test_arm_instruction_reader): New.

    Change-Id: Ieb1c1799bd66f8c7421384f44f5c2777b578ff8d

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26835] undefined behavior in arm_analyze_prologue: shift exponent 32 is too large for 32-bit type 'unsigned int'

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=26835

--- Comment #2 from Simon Marchi <simark at simark dot ca> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26835] undefined behavior in arm_analyze_prologue: shift exponent 32 is too large for 32-bit type 'unsigned int'

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=26835

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug gdb/26835] undefined behavior in arm_analyze_prologue: shift exponent 32 is too large for 32-bit type 'unsigned int'

[luis.machado at arm dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=26835

Luis Machado <luis.machado at arm dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |luis.machado at arm dot com
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #3 from Luis Machado <luis.machado at arm dot com> ---
Fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.