[v2] readelf: Fixes for IN_RANGE

Message ID 1580865.aYqFngRSEh@zbook-opensuse.wgnetz.xx
State New
Headers show
Series
  • [v2] readelf: Fixes for IN_RANGE
Related show

Commit Message

Christian Eggers Nov. 5, 2019, 9:17 p.m.
Hi Nick,

> Hi Christian,

>

> > Various small fixes for the IN_RANGE macro.

>

> Given the size of these patches, it would have been simpler

> just to submit them as one single patch....

done

> Including ChangeLog entries is very nice, but please could you

> provide them as plain text, and not a context diff.  The diff

> almost never applies cleanly because the ChangeLogs are being

> constantly updated.

done

> I think that what we need here is a comment to make it clear what the

> macro is doing.  Something like:


> /* Evaluates to to TRUE if the region ADDR..ADDR+SIZE-1 is contained

> within the address range START..END-1.  */

done

> [...]

> Also note - your mailer has corrupted the context diff, adding "3D" after

> the = sign, and then wrapping the lines at 80 characters.   If you cannot

> turn this behaviour off, then could you provide the patches as attachments,

> rather than inline please.


I sent the patches directly from git via "git send-email". Although the
encoding of the cover letter can be modified via command line, the
individual patch messages were always encoded "quoted printable".

This time I'm sending the patch via kmail according to the instructions
in git-format-patch(1). Even here the mail gets encoded "quoted
printable" if the patch is inline (so I attached it this time).

> Note - this patch is wrong if fix 3/4 is applied as the test needs to check

> for rloc >= end, whereas IN_RANGE would check for rloc > end.  If however

> 3/4 is not applied (as suggested above) then this patch is OK.

Is this issue solved now?

regards
Christian

Comments

Nick Clifton Nov. 6, 2019, 12:32 p.m. | #1
Hi Christian,

  Thanks for the revised patch.  I have now applied it, although I made
  two small changes.  Instead of using "SIZE" as one of the parameters
  I chose to go with "NELEM" on the grounds that this expresses more 
  clearly that it is the number of elements of the type pointed to by
  ADDR.  I also made the comment a little more verbose, just for my
  peace of mind.

Cheers
  Nick

Patch

From d21f2cf92780104ac9f697c7d63b6f56c7333ee4 Mon Sep 17 00:00:00 2001
From: Christian Eggers <ceggers@gmx.de>
Date: Sun, 3 Nov 2019 08:28:38 +0100
Subject: [PATCH] readelf: Various fixes for IN_RANGE

- Rename parameter OFF to SIZE
  All users of IN_RANGE pass the relocation size (rather than the
  offset which is passed in the ADDR parameter).

- Extends Nick Cliftons patch from 2019-08-08 for PR 24829
    e17869d - Catch potential integer overflow in readelf when
    processing corrupt binaries.
  to all address checks using the IN_RANGE macro.

- Fix off by one error whilst checking reloc location against section
  size.

- Use IN_RANGE macro in apply_relocations()

	* readelf.c (IN_RANGE): Rename parameter OFF to SIZE. Add comment.
	Catch potential integer overflow and fix off by one error whilst
	checking reloc location against section size.
	(apply_relocations): Use IN_RANGE macro.

Signed-off-by: Christian Eggers <ceggers@gmx.de>
---
 binutils/readelf.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/binutils/readelf.c b/binutils/readelf.c
index 370bc4c1b7..25afd20a62 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -12309,8 +12309,10 @@  process_syminfo (Filedata * filedata ATTRIBUTE_UNUSED)
   return TRUE;
 }

-#define IN_RANGE(START,END,ADDR,OFF)		\
-  (((ADDR) >= (START)) && ((ADDR) + (OFF) < (END)))
+/* Evaluates to to TRUE if the region ADDR..ADDR+SIZE-1 is contained
+     within the address range START..END-1.  */
+#define IN_RANGE(START,END,ADDR,SIZE)		\
+  (((ADDR) >= (START)) && ((ADDR) < (END)) && ((ADDR) + (SIZE) <= (END)))

 /* Check to see if the given reloc needs to be handled in a target specific
    manner.  If so then process the reloc and return TRUE otherwise return
@@ -13411,7 +13413,7 @@  apply_relocations (Filedata *                 filedata,
 	    }

 	  rloc = start + rp->r_offset;
-	  if (rloc >= end || (rloc + reloc_size) > end || (rloc < start))
+	  if (!IN_RANGE (start, end, rloc, reloc_size))
 	    {
 	      warn (_("skipping invalid relocation offset 0x%lx in section %s\n"),
 		    (unsigned long) rp->r_offset,
--
2.16.4