asan: readelf: process_mips_specific buffer overflow

Message ID 20200611044439.GP8533@bubble.grove.modra.org
State New
Headers show
Series
  • asan: readelf: process_mips_specific buffer overflow
Related show

Commit Message

Alan Modra via Binutils June 11, 2020, 4:44 a.m.
DT_MIPS_OPTIONS is not a regular array as assumed by readelf.  This
patch corrects that assumption, and to do so easily, makes various
internal (host byte order) structs the same size as external (target
byte order) structs.

There is some danger that this patch will result in the readelf
assertions triggering on hosts with weird struct padding rules (I
checked arm and aarch64)..

include/
	* elf/mips.h (Elf32_RegInfo): Use fixed width integer types.
	(Elf64_Internal_RegInfo, Elf_Internal_Options): Likewise.
binutils/
	* readelf.c (process_mips_specific): Assert size of internal
	types match size of external types, and simplify allocation of
	internal buffer.  Catch possible integer overflow when sanity
	checking option size.  Don't assume options are a regular array.
	Sanity check reginfo option against option size.  Use PRI macros
	when printing.


-- 
Alan Modra
Australia Development Lab, IBM

Comments

Alan Modra via Binutils June 11, 2020, 5:23 a.m. | #1
On Thu, Jun 11, 2020 at 02:14:39PM +0930, Alan Modra wrote:
> There is some danger that this patch will result in the readelf

> assertions triggering on hosts with weird struct padding rules (I

> checked arm and aarch64)..


Let's do without that unnecessary internal option buffer.  This also
fixes another bug in that the REGINFO data was being taken from the
calloc'd internal option buffer, so was all zeros.

	* readelf.c (process_mips_specific): Don't alloc memory for
	Elf_Internal_Options.

diff --git a/binutils/readelf.c b/binutils/readelf.c
index 0705a49c0d..101fd66ccb 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -16894,47 +16894,28 @@ process_mips_specific (Filedata * filedata)
                                                 sect->sh_size, _("options"));
       if (eopt)
 	{
-	  Elf_Internal_Options * iopt;
-	  Elf_Internal_Options * option;
-
-	  assert (sizeof (Elf_Internal_Options) == sizeof (Elf_External_Options));
-	  assert (sizeof (Elf32_RegInfo) == sizeof (Elf32_External_RegInfo));
-	  assert (sizeof (Elf64_Internal_RegInfo) == sizeof (Elf64_External_RegInfo));
-	  iopt = (Elf_Internal_Options *) cmalloc (sect->sh_size, 1);
-	  if (iopt == NULL)
-	    {
-	      error (_("Out of memory allocating space for MIPS options\n"));
-	      free (eopt);
-	      return FALSE;
-	    }
+	  Elf_Internal_Options option;
 
 	  offset = cnt = 0;
-	  option = iopt;
-	  
 	  while (offset <= sect->sh_size - sizeof (* eopt))
 	    {
 	      Elf_External_Options * eoption;
+	      unsigned int optsize;
 
 	      eoption = (Elf_External_Options *) ((char *) eopt + offset);
 
-	      option->kind = BYTE_GET (eoption->kind);
-	      option->size = BYTE_GET (eoption->size);
-	      option->section = BYTE_GET (eoption->section);
-	      option->info = BYTE_GET (eoption->info);
+	      optsize = BYTE_GET (eoption->size);
 
 	      /* PR 17531: file: ffa0fa3b.  */
-	      if (option->size < sizeof (* eopt)
-		  || option->size > sect->sh_size - offset)
+	      if (optsize < sizeof (* eopt)
+		  || optsize > sect->sh_size - offset)
 		{
 		  error (_("Invalid size (%u) for MIPS option\n"),
-			 option->size);
-		  free (iopt);
+			 optsize);
 		  free (eopt);
 		  return FALSE;
 		}
-	      offset += option->size;
-
-	      ++option;
+	      offset += optsize;
 	      ++cnt;
 	    }
 
@@ -16947,14 +16928,21 @@ process_mips_specific (Filedata * filedata)
 	  while (cnt-- > 0)
 	    {
 	      size_t len;
+	      Elf_External_Options * eoption;
+
+	      eoption = (Elf_External_Options *) ((char *) eopt + offset);
+
+	      option.kind = BYTE_GET (eoption->kind);
+	      option.size = BYTE_GET (eoption->size);
+	      option.section = BYTE_GET (eoption->section);
+	      option.info = BYTE_GET (eoption->info);
 
-	      option = (Elf_Internal_Options *) ((char *) iopt + offset);
-	      switch (option->kind)
+	      switch (option.kind)
 		{
 		case ODK_NULL:
 		  /* This shouldn't happen.  */
 		  printf (" NULL       %" PRId16 " %" PRIx32,
-			  option->section, option->info);
+			  option.section, option.info);
 		  break;
 
 		case ODK_REGINFO:
@@ -16965,8 +16953,8 @@ process_mips_specific (Filedata * filedata)
 		      Elf32_RegInfo reginfo;
 
 		      /* 32bit form.  */
-		      if (option->size < (sizeof (Elf_External_Options)
-					  + sizeof (Elf32_External_RegInfo)))
+		      if (option.size < (sizeof (Elf_External_Options)
+					 + sizeof (Elf32_External_RegInfo)))
 			{
 			  printf (_("<corrupt>\n"));
 			  error (_("Truncated MIPS REGINFO option\n"));
@@ -16974,7 +16962,7 @@ process_mips_specific (Filedata * filedata)
 			  break;
 			}
 
-		      ereg = (Elf32_External_RegInfo *) (option + 1);
+		      ereg = (Elf32_External_RegInfo *) (eoption + 1);
 
 		      reginfo.ri_gprmask = BYTE_GET (ereg->ri_gprmask);
 		      reginfo.ri_cprmask[0] = BYTE_GET (ereg->ri_cprmask[0]);
@@ -16997,8 +16985,8 @@ process_mips_specific (Filedata * filedata)
 		      Elf64_External_RegInfo * ereg;
 		      Elf64_Internal_RegInfo reginfo;
 
-		      if (option->size < (sizeof (Elf_External_Options)
-					  + sizeof (Elf64_External_RegInfo)))
+		      if (option.size < (sizeof (Elf_External_Options)
+					 + sizeof (Elf64_External_RegInfo)))
 			{
 			  printf (_("<corrupt>\n"));
 			  error (_("Truncated MIPS REGINFO option\n"));
@@ -17006,7 +16994,7 @@ process_mips_specific (Filedata * filedata)
 			  break;
 			}
 
-		      ereg = (Elf64_External_RegInfo *) (option + 1);
+		      ereg = (Elf64_External_RegInfo *) (eoption + 1);
 		      reginfo.ri_gprmask    = BYTE_GET (ereg->ri_gprmask);
 		      reginfo.ri_cprmask[0] = BYTE_GET (ereg->ri_cprmask[0]);
 		      reginfo.ri_cprmask[1] = BYTE_GET (ereg->ri_cprmask[1]);
@@ -17022,45 +17010,45 @@ process_mips_specific (Filedata * filedata)
 			      reginfo.ri_cprmask[0], reginfo.ri_cprmask[1],
 			      reginfo.ri_cprmask[2], reginfo.ri_cprmask[3]);
 		    }
-		  offset += option->size;
+		  offset += option.size;
 		  continue;
 
 		case ODK_EXCEPTIONS:
 		  fputs (" EXCEPTIONS fpe_min(", stdout);
-		  process_mips_fpe_exception (option->info & OEX_FPU_MIN);
+		  process_mips_fpe_exception (option.info & OEX_FPU_MIN);
 		  fputs (") fpe_max(", stdout);
-		  process_mips_fpe_exception ((option->info & OEX_FPU_MAX) >> 8);
+		  process_mips_fpe_exception ((option.info & OEX_FPU_MAX) >> 8);
 		  fputs (")", stdout);
 
-		  if (option->info & OEX_PAGE0)
+		  if (option.info & OEX_PAGE0)
 		    fputs (" PAGE0", stdout);
-		  if (option->info & OEX_SMM)
+		  if (option.info & OEX_SMM)
 		    fputs (" SMM", stdout);
-		  if (option->info & OEX_FPDBUG)
+		  if (option.info & OEX_FPDBUG)
 		    fputs (" FPDBUG", stdout);
-		  if (option->info & OEX_DISMISS)
+		  if (option.info & OEX_DISMISS)
 		    fputs (" DISMISS", stdout);
 		  break;
 
 		case ODK_PAD:
 		  fputs (" PAD       ", stdout);
-		  if (option->info & OPAD_PREFIX)
+		  if (option.info & OPAD_PREFIX)
 		    fputs (" PREFIX", stdout);
-		  if (option->info & OPAD_POSTFIX)
+		  if (option.info & OPAD_POSTFIX)
 		    fputs (" POSTFIX", stdout);
-		  if (option->info & OPAD_SYMBOL)
+		  if (option.info & OPAD_SYMBOL)
 		    fputs (" SYMBOL", stdout);
 		  break;
 
 		case ODK_HWPATCH:
 		  fputs (" HWPATCH   ", stdout);
-		  if (option->info & OHW_R4KEOP)
+		  if (option.info & OHW_R4KEOP)
 		    fputs (" R4KEOP", stdout);
-		  if (option->info & OHW_R8KPFETCH)
+		  if (option.info & OHW_R8KPFETCH)
 		    fputs (" R8KPFETCH", stdout);
-		  if (option->info & OHW_R5KEOP)
+		  if (option.info & OHW_R5KEOP)
 		    fputs (" R5KEOP", stdout);
-		  if (option->info & OHW_R5KCVTL)
+		  if (option.info & OHW_R5KCVTL)
 		    fputs (" R5KCVTL", stdout);
 		  break;
 
@@ -17076,43 +17064,43 @@ process_mips_specific (Filedata * filedata)
 
 		case ODK_HWAND:
 		  fputs (" HWAND     ", stdout);
-		  if (option->info & OHWA0_R4KEOP_CHECKED)
+		  if (option.info & OHWA0_R4KEOP_CHECKED)
 		    fputs (" R4KEOP_CHECKED", stdout);
-		  if (option->info & OHWA0_R4KEOP_CLEAN)
+		  if (option.info & OHWA0_R4KEOP_CLEAN)
 		    fputs (" R4KEOP_CLEAN", stdout);
 		  break;
 
 		case ODK_HWOR:
 		  fputs (" HWOR      ", stdout);
-		  if (option->info & OHWA0_R4KEOP_CHECKED)
+		  if (option.info & OHWA0_R4KEOP_CHECKED)
 		    fputs (" R4KEOP_CHECKED", stdout);
-		  if (option->info & OHWA0_R4KEOP_CLEAN)
+		  if (option.info & OHWA0_R4KEOP_CLEAN)
 		    fputs (" R4KEOP_CLEAN", stdout);
 		  break;
 
 		case ODK_GP_GROUP:
 		  printf (" GP_GROUP  %#06x  self-contained %#06x",
-			  option->info & OGP_GROUP,
-			  (option->info & OGP_SELF) >> 16);
+			  option.info & OGP_GROUP,
+			  (option.info & OGP_SELF) >> 16);
 		  break;
 
 		case ODK_IDENT:
 		  printf (" IDENT     %#06x  self-contained %#06x",
-			  option->info & OGP_GROUP,
-			  (option->info & OGP_SELF) >> 16);
+			  option.info & OGP_GROUP,
+			  (option.info & OGP_SELF) >> 16);
 		  break;
 
 		default:
 		  /* This shouldn't happen.  */
 		  printf (" %3d ???     %" PRId16 " %" PRIx32,
-			  option->kind, option->section, option->info);
+			  option.kind, option.section, option.info);
 		  break;
 		}
 
 	      len = sizeof (* eopt);
-	      while (len < option->size)
+	      while (len < option.size)
 		{
-		  unsigned char datum = * ((unsigned char *) eopt + offset + len);
+		  unsigned char datum = *((unsigned char *) eoption + len);
 
 		  if (ISPRINT (datum))
 		    printf ("%c", datum);
@@ -17122,9 +17110,8 @@ process_mips_specific (Filedata * filedata)
 		}
 	      fputs ("\n", stdout);
 
-	      offset += option->size;
+	      offset += option.size;
 	    }
-	  free (iopt);
 	  free (eopt);
 	}
       else


-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/binutils/readelf.c b/binutils/readelf.c
index 0bdabccc8e..0705a49c0d 100644
--- a/binutils/readelf.c
+++ b/binutils/readelf.c
@@ -16896,10 +16896,11 @@  process_mips_specific (Filedata * filedata)
 	{
 	  Elf_Internal_Options * iopt;
 	  Elf_Internal_Options * option;
-	  Elf_Internal_Options * iopt_end;
 
-	  iopt = (Elf_Internal_Options *)
-              cmalloc ((sect->sh_size / sizeof (eopt)), sizeof (* iopt));
+	  assert (sizeof (Elf_Internal_Options) == sizeof (Elf_External_Options));
+	  assert (sizeof (Elf32_RegInfo) == sizeof (Elf32_External_RegInfo));
+	  assert (sizeof (Elf64_Internal_RegInfo) == sizeof (Elf64_External_RegInfo));
+	  iopt = (Elf_Internal_Options *) cmalloc (sect->sh_size, 1);
 	  if (iopt == NULL)
 	    {
 	      error (_("Out of memory allocating space for MIPS options\n"));
@@ -16909,7 +16910,6 @@  process_mips_specific (Filedata * filedata)
 
 	  offset = cnt = 0;
 	  option = iopt;
-	  iopt_end = iopt + (sect->sh_size / sizeof (eopt));
 	  
 	  while (offset <= sect->sh_size - sizeof (* eopt))
 	    {
@@ -16924,7 +16924,7 @@  process_mips_specific (Filedata * filedata)
 
 	      /* PR 17531: file: ffa0fa3b.  */
 	      if (option->size < sizeof (* eopt)
-		  || offset + option->size > sect->sh_size)
+		  || option->size > sect->sh_size - offset)
 		{
 		  error (_("Invalid size (%u) for MIPS option\n"),
 			 option->size);
@@ -16943,18 +16943,18 @@  process_mips_specific (Filedata * filedata)
 			    cnt),
 		  printable_section_name (filedata, sect), cnt);
 
-	  option = iopt;
 	  offset = 0;
-
 	  while (cnt-- > 0)
 	    {
 	      size_t len;
 
+	      option = (Elf_Internal_Options *) ((char *) iopt + offset);
 	      switch (option->kind)
 		{
 		case ODK_NULL:
 		  /* This shouldn't happen.  */
-		  printf (" NULL       %d %lx", option->section, option->info);
+		  printf (" NULL       %" PRId16 " %" PRIx32,
+			  option->section, option->info);
 		  break;
 
 		case ODK_REGINFO:
@@ -16965,7 +16965,8 @@  process_mips_specific (Filedata * filedata)
 		      Elf32_RegInfo reginfo;
 
 		      /* 32bit form.  */
-		      if (option + 2 > iopt_end)
+		      if (option->size < (sizeof (Elf_External_Options)
+					  + sizeof (Elf32_External_RegInfo)))
 			{
 			  printf (_("<corrupt>\n"));
 			  error (_("Truncated MIPS REGINFO option\n"));
@@ -16982,10 +16983,11 @@  process_mips_specific (Filedata * filedata)
 		      reginfo.ri_cprmask[3] = BYTE_GET (ereg->ri_cprmask[3]);
 		      reginfo.ri_gp_value = BYTE_GET (ereg->ri_gp_value);
 
-		      printf ("GPR %08lx  GP 0x%lx\n",
-			      reginfo.ri_gprmask,
-			      (unsigned long) reginfo.ri_gp_value);
-		      printf ("            CPR0 %08lx  CPR1 %08lx  CPR2 %08lx  CPR3 %08lx\n",
+		      printf ("GPR %08" PRIx32 "  GP 0x%" PRIx32 "\n",
+			      reginfo.ri_gprmask, reginfo.ri_gp_value);
+		      printf ("          "
+			      "  CPR0 %08" PRIx32 "  CPR1 %08" PRIx32
+			      "  CPR2 %08" PRIx32 "  CPR3 %08" PRIx32 "\n",
 			      reginfo.ri_cprmask[0], reginfo.ri_cprmask[1],
 			      reginfo.ri_cprmask[2], reginfo.ri_cprmask[3]);
 		    }
@@ -16995,7 +16997,8 @@  process_mips_specific (Filedata * filedata)
 		      Elf64_External_RegInfo * ereg;
 		      Elf64_Internal_RegInfo reginfo;
 
-		      if (option + 2 > iopt_end)
+		      if (option->size < (sizeof (Elf_External_Options)
+					  + sizeof (Elf64_External_RegInfo)))
 			{
 			  printf (_("<corrupt>\n"));
 			  error (_("Truncated MIPS REGINFO option\n"));
@@ -17011,16 +17014,15 @@  process_mips_specific (Filedata * filedata)
 		      reginfo.ri_cprmask[3] = BYTE_GET (ereg->ri_cprmask[3]);
 		      reginfo.ri_gp_value   = BYTE_GET (ereg->ri_gp_value);
 
-		      printf ("GPR %08lx  GP 0x",
-			      reginfo.ri_gprmask);
-		      printf_vma (reginfo.ri_gp_value);
-		      printf ("\n");
-
-		      printf ("            CPR0 %08lx  CPR1 %08lx  CPR2 %08lx  CPR3 %08lx\n",
+		      printf ("GPR %08" PRIx32 "  GP 0x%" PRIx64 "\n",
+			      reginfo.ri_gprmask, reginfo.ri_gp_value);
+		      printf ("          "
+			      "  CPR0 %08" PRIx32 "  CPR1 %08" PRIx32
+			      "  CPR2 %08" PRIx32 "  CPR3 %08" PRIx32 "\n",
 			      reginfo.ri_cprmask[0], reginfo.ri_cprmask[1],
 			      reginfo.ri_cprmask[2], reginfo.ri_cprmask[3]);
 		    }
-		  ++option;
+		  offset += option->size;
 		  continue;
 
 		case ODK_EXCEPTIONS:
@@ -17089,20 +17091,20 @@  process_mips_specific (Filedata * filedata)
 		  break;
 
 		case ODK_GP_GROUP:
-		  printf (" GP_GROUP  %#06lx  self-contained %#06lx",
+		  printf (" GP_GROUP  %#06x  self-contained %#06x",
 			  option->info & OGP_GROUP,
 			  (option->info & OGP_SELF) >> 16);
 		  break;
 
 		case ODK_IDENT:
-		  printf (" IDENT     %#06lx  self-contained %#06lx",
+		  printf (" IDENT     %#06x  self-contained %#06x",
 			  option->info & OGP_GROUP,
 			  (option->info & OGP_SELF) >> 16);
 		  break;
 
 		default:
 		  /* This shouldn't happen.  */
-		  printf (" %3d ???     %d %lx",
+		  printf (" %3d ???     %" PRId16 " %" PRIx32,
 			  option->kind, option->section, option->info);
 		  break;
 		}
@@ -17121,7 +17123,6 @@  process_mips_specific (Filedata * filedata)
 	      fputs ("\n", stdout);
 
 	      offset += option->size;
-	      ++option;
 	    }
 	  free (iopt);
 	  free (eopt);
diff --git a/include/elf/mips.h b/include/elf/mips.h
index d116b036b6..cc08ebd431 100644
--- a/include/elf/mips.h
+++ b/include/elf/mips.h
@@ -560,11 +560,11 @@  typedef union
 typedef struct
 {
   /* Mask of general purpose registers used.  */
-  unsigned long ri_gprmask;
+  uint32_t ri_gprmask;
   /* Mask of co-processor registers used.  */
-  unsigned long ri_cprmask[4];
+  uint32_t ri_cprmask[4];
   /* GP register value for this object file.  */
-  long ri_gp_value;
+  uint32_t ri_gp_value;
 } Elf32_RegInfo;
 
 /* The external version of the Elf_RegInfo structure.  */
@@ -1008,9 +1008,9 @@  typedef struct
   /* Size of option descriptor, including header.  */
   unsigned char size;
   /* Section index of affected section, or 0 for global option.  */
-  unsigned short section;
+  uint16_t section;
   /* Information specific to this kind of option.  */
-  unsigned long info;
+  uint32_t info;
 } Elf_Internal_Options;
 
 /* MIPS ELF option header swapping routines.  */
@@ -1074,13 +1074,13 @@  typedef struct
 typedef struct
 {
   /* Mask of general purpose registers used.  */
-  unsigned long ri_gprmask;
+  uint32_t ri_gprmask;
   /* Padding.  */
-  unsigned long ri_pad;
+  uint32_t ri_pad;
   /* Mask of co-processor registers used.  */
-  unsigned long ri_cprmask[4];
+  uint32_t ri_cprmask[4];
   /* GP register value for this object file.  */
-  bfd_vma ri_gp_value;
+  uint64_t ri_gp_value;
 } Elf64_Internal_RegInfo;
 
 /* ABI Flags structure version 0.  */