: Fix range check in ihex PR/24065 for 32 bit hosts.

Message ID 20190108101516.GA16370@arm.com
State New
Headers show
Series
  • : Fix range check in ihex PR/24065 for 32 bit hosts.
Related show

Commit Message

Tamar Christina Jan. 8, 2019, 10:15 a.m.
Hi All,

The change in PR binutils/24065 added a check to see if the value is in a 32-bit range.
It does this using two 64-bit masks that are stored inside a bfd_vma.

This now rejects all values on a 32-bit host as bfd_vma seems host dependent.  This
patch changes the type for the masks from bfd_vma to uint64_t.
    
build on native hardware and regtested on
  aarch64-none-elf, aarch64-none-elf (32 bit host),
  aarch64-none-linux-gnu, aarch64-none-linux-gnu (32 bit host)
  arm-none-eabi, arm-none-eabi (32 bit host),
  arm-none-linux-gnueabihf, arm-none-linux-gnueabihf (32 bit host)

Cross-compiled and regtested on
  aarch64-none-linux-gnu, aarch64_be-none-linux-gnu
  arm-none-eabi, armeb-none-eabi

and no issues.

Ok for master?

Thanks,
Tamar

bfd/ChangeLog:

2019-01-08  Tamar Christina  <tamar.christina@arm.com>

	PR binutils/24065
	* ihex.c (ihex_write_object_contents): Change bfd_vma to uint64_t.

binutils/ChangeLog:

2019-01-08  Tamar Christina  <tamar.christina@arm.com>

	PR binutils/24065
	* testsuite/binutils-all/copy-6.d: New test.
	* testsuite/binutils-all/objcopy.exp: Use it.

--

Comments

Tamar Christina Jan. 8, 2019, 10:21 a.m. | #1
Oops, screwed up the PR in the cover letter, The original change was added by PR binutils/23699.

> -----Original Message-----

> From: binutils-owner@sourceware.org <binutils-owner@sourceware.org>

> On Behalf Of Tamar Christina

> Sent: Tuesday, January 8, 2019 10:15

> To: binutils@sourceware.org

> Cc: nd <nd@arm.com>

> Subject: [PATCH][Binutils]: Fix range check in ihex PR/24065 for 32 bit hosts.

> 

> Hi All,

> 

> The change in PR binutils/24065 added a check to see if the value is in a 32-bit

> range.

> It does this using two 64-bit masks that are stored inside a bfd_vma.

> 

> This now rejects all values on a 32-bit host as bfd_vma seems host

> dependent.  This patch changes the type for the masks from bfd_vma to

> uint64_t.

> 

> build on native hardware and regtested on

>   aarch64-none-elf, aarch64-none-elf (32 bit host),

>   aarch64-none-linux-gnu, aarch64-none-linux-gnu (32 bit host)

>   arm-none-eabi, arm-none-eabi (32 bit host),

>   arm-none-linux-gnueabihf, arm-none-linux-gnueabihf (32 bit host)

> 

> Cross-compiled and regtested on

>   aarch64-none-linux-gnu, aarch64_be-none-linux-gnu

>   arm-none-eabi, armeb-none-eabi

> 

> and no issues.

> 

> Ok for master?

> 

> Thanks,

> Tamar

> 

> bfd/ChangeLog:

> 

> 2019-01-08  Tamar Christina  <tamar.christina@arm.com>

> 

> 	PR binutils/24065

> 	* ihex.c (ihex_write_object_contents): Change bfd_vma to uint64_t.

> 

> binutils/ChangeLog:

> 

> 2019-01-08  Tamar Christina  <tamar.christina@arm.com>

> 

> 	PR binutils/24065

> 	* testsuite/binutils-all/copy-6.d: New test.

> 	* testsuite/binutils-all/objcopy.exp: Use it.

> 

> --
Alan Modra Jan. 8, 2019, noon | #2
On Tue, Jan 08, 2019 at 10:15:20AM +0000, Tamar Christina wrote:
> The change in PR binutils/24065 added a check to see if the value is in a 32-bit range.

> It does this using two 64-bit masks that are stored inside a bfd_vma.


Which isn't the cleanest way to check.

> This now rejects all values on a 32-bit host as bfd_vma seems host dependent.


Right.  So let's fix the logic here rather than forcing 64-bit
compares.

	PR 23699
	PR 24065
	* ihex.c (ihex_write_object_contents): Properly check 32-bit
	address range.

diff --git a/bfd/ihex.c b/bfd/ihex.c
index 5d7d8fffea..101e0a7615 100644
--- a/bfd/ihex.c
+++ b/bfd/ihex.c
@@ -775,25 +775,29 @@ ihex_write_object_contents (bfd *abfd)
       bfd_vma where;
       bfd_byte *p;
       bfd_size_type count;
-      const bfd_vma sign = (bfd_vma) 0xffffffff80000000ULL;
-      const bfd_vma top = (bfd_vma) 0xffffffff00000000ULL;
 
       where = l->where;
 
-      /* Check for unacceptable addresses sign extension.
-	 See PR 23699 for more details.  */
-      if ((where & sign) == top
-	  || ((where & top) != 0 && (where & top) != top))
-       {
-         _bfd_error_handler
-           /* xgettext:c-format */
-           (_("%pB 64-bit address %#" PRIx64 " out of range for Intel Hex file"),
-            abfd, (uint64_t) where);
-         bfd_set_error (bfd_error_bad_value);
-         return FALSE;
-       }
-
+#ifdef BFD64
+      /* IHex only supports 32-bit addresses, and we want to check
+	 that 64-bit addresses are in range.  This isn't quite as
+	 obvious as it may seem, since some targets have 32-bit
+	 addresses that are sign extended to 64 bits.  So complain
+	 only if addresses overflow both unsigned and signed 32-bit
+	 integers.  */
+      if (where > 0xffffffff
+	  && where + 0x80000000 > 0xffffffff)
+	{
+	  _bfd_error_handler
+	    /* xgettext:c-format */
+	    (_("%pB 64-bit address %#" PRIx64
+	       " out of range for Intel Hex file"),
+	     abfd, (uint64_t) where);
+	  bfd_set_error (bfd_error_bad_value);
+	  return FALSE;
+	}
       where &= 0xffffffff;
+#endif
 
       p = l->data;
       count = l->size;

-- 
Alan Modra
Australia Development Lab, IBM
Alan Modra Jan. 8, 2019, 1:57 p.m. | #3
> 	PR binutils/24065

> 	* testsuite/binutils-all/copy-6.d: New test.

> 	* testsuite/binutils-all/objcopy.exp: Use it.


I forgot to say, this part is OK

> +#...

> \ No newline at end of file


but please do fix this before committing.

-- 
Alan Modra
Australia Development Lab, IBM
Tamar Christina Jan. 8, 2019, 2:52 p.m. | #4
Hi Alan,

> 

> I forgot to say, this part is OK

> 

> > +#...

> > \ No newline at end of file

> 

> but please do fix this before committing.

> 


I've fixed it and will commit the patch with just the test.

Thanks,
Tamar

> --

> Alan Modra

> Australia Development Lab, IBM

Patch

diff --git a/bfd/ihex.c b/bfd/ihex.c
index 5d7d8fffeaaabc992e3923866e679e7e1daaf644..d5097f37307fcfb7cf757d6b215d53f20dd82e16 100644
--- a/bfd/ihex.c
+++ b/bfd/ihex.c
@@ -775,8 +775,8 @@  ihex_write_object_contents (bfd *abfd)
       bfd_vma where;
       bfd_byte *p;
       bfd_size_type count;
-      const bfd_vma sign = (bfd_vma) 0xffffffff80000000ULL;
-      const bfd_vma top = (bfd_vma) 0xffffffff00000000ULL;
+      const uint64_t sign = (uint64_t) 0xffffffff80000000ULL;
+      const uint64_t top = (uint64_t) 0xffffffff00000000ULL;
 
       where = l->where;
 
diff --git a/binutils/testsuite/binutils-all/copy-6.d b/binutils/testsuite/binutils-all/copy-6.d
new file mode 100644
index 0000000000000000000000000000000000000000..ec8365379eb1c0ebc5a0a68f7516748569900bff
--- /dev/null
+++ b/binutils/testsuite/binutils-all/copy-6.d
@@ -0,0 +1,8 @@ 
+#PROG: objcopy
+#source: bintest.s
+#objcopy: -O ihex
+# A few targets cannot assemble the bintest.s source file...
+#notarget: pdp11-* *-darwin
+#name: ihex objcopy test
+#objdump: -h
+#...
\ No newline at end of file
diff --git a/binutils/testsuite/binutils-all/objcopy.exp b/binutils/testsuite/binutils-all/objcopy.exp
index d8bfca8c11c6058b4e5771bc1c5807cfd938ee4f..921e6a23c4f2f3c12169d8941d1c4f7912589e81 100644
--- a/binutils/testsuite/binutils-all/objcopy.exp
+++ b/binutils/testsuite/binutils-all/objcopy.exp
@@ -1076,6 +1076,7 @@  run_dump_test "copy-2"
 run_dump_test "copy-3"
 run_dump_test "copy-4"
 run_dump_test "copy-5"
+run_dump_test "copy-6"
 
 # Use bintest.o from the copy-4 test to determine ELF reloc type
 set reloc_format rel