Re: [PATCH][Binutils][Arm] gas: fix out of range conditional branch (PR/24991)

[Tamar Christina <Tamar dot Christina at arm dot com>]

Hi Nick,

Attached is updated patch which should address your and Andreas Comments.


2019-09-24  Tamar Christina  <tamar.christina@arm.com>

	PR gas/24991
	* config/tc-arm.c (out_of_range_p): New.
	(md_apply_fix): Use it in BFD_RELOC_THUMB_PCREL_BRANCH9,
	BFD_RELOC_THUMB_PCREL_BRANCH12, BFD_RELOC_THUMB_PCREL_BRANCH20,
	BFD_RELOC_THUMB_PCREL_BRANCH23, BFD_RELOC_THUMB_PCREL_BRANCH25
	* testsuite/gas/arm/pr24991.d: New test.
	* testsuite/gas/arm/pr24991.l: New test.
	* testsuite/gas/arm/pr24991.s: New test.

Ok for master?

Thanks,
Tamar

The 09/23/2019 11:06, Nick Clifton wrote:
> Hi Tamar,
> 
>   Being paranoid here....
> 
> > +/* Perform range checks on positive and negative overflows by checking if the
> > +   VALUE given fits within the range of an BITS sized immediate.  */
> > +static bfd_boolean out_of_range_p (offsetT value, bfd_vma bits)
> > +{
> > +  return (value & ~((1 << bits)-1))
> > +         && ((value & ~((1 << bits)-1)) != ~((1 << bits)-1));
> > +}
> 
> If bits is a large number then these shifts could overflow.  I would
> recommend adding a range check first.  Plus you should allow for the
> fact that sizeof (bits) and sizeof (value) could be small if the code
> is compiled for a 32-bit host...
> 
> Cheers
>   Nick
> 
> 
> 

-- 

diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
index 32a15f65ec852bae54184ab29447f17ab84d8fec..de0510383569b0e9cb389a7a9f2d6635a7ae3543 100644
--- a/gas/config/tc-arm.c
+++ b/gas/config/tc-arm.c
@@ -106,6 +106,15 @@ enum arm_float_abi
    should define CPU_DEFAULT here.  */
 #endif
 
+/* Perform range checks on positive and negative overflows by checking if the
+   VALUE given fits within the range of an BITS sized immediate.  */
+static bfd_boolean out_of_range_p (offsetT value, offsetT bits)
+ {
+  gas_assert (bits < (offsetT)(sizeof (value) * 8));
+  return (value & ~((1 << bits)-1))
+	  && ((value & ~((1 << bits)-1)) != ~((1 << bits)-1));
+}
+
 #ifndef FPU_DEFAULT
 # ifdef TE_LINUX
 #  define FPU_DEFAULT FPU_ARCH_FPA
@@ -28088,7 +28097,7 @@ md_apply_fix (fixS *	fixP,
       break;
 
     case BFD_RELOC_THUMB_PCREL_BRANCH9: /* Conditional branch.	*/
-      if ((value & ~0xff) && ((value & ~0xff) != ~0xff))
+      if (out_of_range_p (value, 8))
 	as_bad_where (fixP->fx_file, fixP->fx_line, BAD_RANGE);
 
       if (fixP->fx_done || !seg->use_rela_p)
@@ -28100,7 +28109,7 @@ md_apply_fix (fixS *	fixP,
       break;
 
     case BFD_RELOC_THUMB_PCREL_BRANCH12: /* Unconditional branch.  */
-      if ((value & ~0x7ff) && ((value & ~0x7ff) != ~0x7ff))
+      if (out_of_range_p (value, 11))
 	as_bad_where (fixP->fx_file, fixP->fx_line, BAD_RANGE);
 
       if (fixP->fx_done || !seg->use_rela_p)
@@ -28111,6 +28120,7 @@ md_apply_fix (fixS *	fixP,
 	}
       break;
 
+    /* This relocation is misnamed, it should be BRANCH21.  */
     case BFD_RELOC_THUMB_PCREL_BRANCH20:
       if (fixP->fx_addsy
 	  && (S_GET_SEGMENT (fixP->fx_addsy) == seg)
@@ -28121,7 +28131,7 @@ md_apply_fix (fixS *	fixP,
 	  /* Force a relocation for a branch 20 bits wide.  */
 	  fixP->fx_done = 0;
 	}
-      if ((value & ~0x1fffff) && ((value & ~0x0fffff) != ~0x0fffff))
+      if (out_of_range_p (value, 20))
 	as_bad_where (fixP->fx_file, fixP->fx_line,
 		      _("conditional branch out of range"));
 
@@ -28200,12 +28210,11 @@ md_apply_fix (fixS *	fixP,
 	 fixP->fx_r_type = BFD_RELOC_THUMB_PCREL_BRANCH23;
 #endif
 
-      if ((value & ~0x3fffff) && ((value & ~0x3fffff) != ~0x3fffff))
+      if (out_of_range_p (value, 22))
 	{
 	  if (!(ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v6t2)))
 	    as_bad_where (fixP->fx_file, fixP->fx_line, BAD_RANGE);
-	  else if ((value & ~0x1ffffff)
-		   && ((value & ~0x1ffffff) != ~0x1ffffff))
+	  else if (out_of_range_p (value, 24))
 	    as_bad_where (fixP->fx_file, fixP->fx_line,
 			  _("Thumb2 branch out of range"));
 	}
@@ -28216,7 +28225,7 @@ md_apply_fix (fixS *	fixP,
       break;
 
     case BFD_RELOC_THUMB_PCREL_BRANCH25:
-      if ((value & ~0x0ffffff) && ((value & ~0x0ffffff) != ~0x0ffffff))
+      if (out_of_range_p (value, 24))
 	as_bad_where (fixP->fx_file, fixP->fx_line, BAD_RANGE);
 
       if (fixP->fx_done || !seg->use_rela_p)
diff --git a/gas/testsuite/gas/arm/pr24991.d b/gas/testsuite/gas/arm/pr24991.d
new file mode 100644
index 0000000000000000000000000000000000000000..2acca2d656ccfa9233dcdf12a35633a642d41fbe
--- /dev/null
+++ b/gas/testsuite/gas/arm/pr24991.d
@@ -0,0 +1,4 @@
+#as: -march=armv7-a
+#source: pr24991.s
+#error_output: pr24991.l
+
diff --git a/gas/testsuite/gas/arm/pr24991.l b/gas/testsuite/gas/arm/pr24991.l
new file mode 100644
index 0000000000000000000000000000000000000000..4fc58751c8dfd866f5dc6e361434b0210c807e85
--- /dev/null
+++ b/gas/testsuite/gas/arm/pr24991.l
@@ -0,0 +1,2 @@
+[^:]*: Assembler messages:
+[^:]*:4: Error: conditional branch out of range
diff --git a/gas/testsuite/gas/arm/pr24991.s b/gas/testsuite/gas/arm/pr24991.s
new file mode 100644
index 0000000000000000000000000000000000000000..27f8daff1c81bc31dd5630f9dcff5c664d8e845f
--- /dev/null
+++ b/gas/testsuite/gas/arm/pr24991.s
@@ -0,0 +1,5 @@
+        .arch armv7-a
+        .syntax unified
+        .thumb
+        beq     .+ 0x124f80
+