PATCH: Fix buffer overflow in gas

PATCH: Fix buffer overflow in gas

[H. J. Lu]

There are some potential buffer overflows in gas. 8byte isn't enough
to hold a negative byte. This patch fixes them. Also we should use
snprintf instead of sprintf.


H.J.
---
2006-05-01  H.J. Lu  <hongjiu.lu@intel.com>

	* config/tc-i386.c (output_invalid_buf): Change size to 16.
	* config/tc-tic30.c (output_invalid_buf): Likewise.

	* config/tc-i386.c (output_invalid): Use snprintf instead of
	sprintf.
	* config/tc-ia64.c (declare_register_set): Likewise.
	(emit_one_bundle): Likewise.
	(check_dependencies): Likewise.
	* config/tc-tic30.c (output_invalid): Likewise.

--- gas/config/tc-i386.c.buf	2006-04-25 14:35:46.000000000 -0700
+++ gas/config/tc-i386.c	2006-05-01 11:13:22.000000000 -0700
@@ -5251,16 +5251,18 @@ md_atof (type, litP, sizeP)
   return 0;
 }
 \f
-static char output_invalid_buf[8];
+static char output_invalid_buf[16];
 
 static char *
 output_invalid (c)
      int c;
 {
   if (ISPRINT (c))
-    sprintf (output_invalid_buf, "'%c'", c);
+    snprintf (output_invalid_buf, sizeof (output_invalid_buf),
+	      "'%c'", c);
   else
-    sprintf (output_invalid_buf, "(0x%x)", (unsigned) c);
+    snprintf (output_invalid_buf, sizeof (output_invalid_buf),
+	      "(0x%x)", (unsigned) c);
   return output_invalid_buf;
 }
 
--- gas/config/tc-ia64.c.buf	2006-04-25 14:35:46.000000000 -0700
+++ gas/config/tc-ia64.c	2006-05-01 11:26:49.000000000 -0700
@@ -5634,7 +5634,7 @@ declare_register_set (prefix, num_regs, 
 
   for (i = 0; i < num_regs; ++i)
     {
-      sprintf (name, "%s%u", prefix, i);
+      snprintf (name, sizeof (name), "%s%u", prefix, i);
       declare_register (name, base_regnum + i);
     }
 }
@@ -6971,7 +6971,8 @@ emit_one_bundle ()
 	  else
 	    as_fatal ("emit_one_bundle: unexpected dynamic op");
 
-	  sprintf (mnemonic, "%s.%c", idesc->name, "?imbfxx"[insn_unit]);
+	  snprintf (mnemonic, sizeof (mnemonic), "%s.%c",
+		    idesc->name, "?imbfxx"[insn_unit]);
 	  opnd1 = idesc->operands[0];
 	  opnd2 = idesc->operands[1];
 	  ia64_free_opcode (idesc);
@@ -10544,12 +10545,15 @@ check_dependencies (idesc)
 	      int certain = (matchtype == 1 && CURR_SLOT.qp_regno == 0);
 
 	      if (path != 0)
-		sprintf (pathmsg, " when entry is at label '%s'",
+		snprintf (pathmsg, sizeof (pathmsg),
+			  " when entry is at label '%s'",
 			 md.entry_labels[path - 1]);
 	      if (matchtype == 1 && rs->index >= 0)
-		sprintf (indexmsg, ", specific resource number is %d",
+		snprintf (indexmsg, sizeof (indexmsg),
+			  ", specific resource number is %d",
 			 rs->index);
-	      sprintf (msg, "Use of '%s' %s %s dependency '%s' (%s)%s%s",
+	      snprintf (msg, sizeof (msg),
+			"Use of '%s' %s %s dependency '%s' (%s)%s%s",
 		       idesc->name,
 		       (certain ? "violates" : "may violate"),
 		       dv_mode[dep->mode], dep->name,
--- gas/config/tc-tic30.c.buf	2005-08-15 07:50:53.000000000 -0700
+++ gas/config/tc-tic30.c	2006-05-01 11:13:53.000000000 -0700
@@ -273,15 +273,17 @@ struct tic30_insn
 struct tic30_insn insn;
 static int found_parallel_insn;
 
-static char output_invalid_buf[8];
+static char output_invalid_buf[16];
 
 static char *
 output_invalid (char c)
 {
   if (ISPRINT (c))
-    sprintf (output_invalid_buf, "'%c'", c);
+    snprintf (output_invalid_buf, sizeof (output_invalid_buf),
+	      "'%c'", c);
   else
-    sprintf (output_invalid_buf, "(0x%x)", (unsigned) c);
+    snprintf (output_invalid_buf, sizeof (output_invalid_buf), 
+	      "(0x%x)", (unsigned) c);
   return output_invalid_buf;
 }
 

Re: PATCH: Fix buffer overflow in gas

[Nick Clifton]

Hi H. J.

> There are some potential buffer overflows in gas. 8byte isn't enough
> to hold a negative byte. This patch fixes them. Also we should use
> snprintf instead of sprintf.

Did you test this patch ?  if so, please could you say how.

> 2006-05-01  H.J. Lu  <hongjiu.lu@intel.com>
> 
> 	* config/tc-i386.c (output_invalid_buf): Change size to 16.
> 	* config/tc-tic30.c (output_invalid_buf): Likewise.
> 
> 	* config/tc-i386.c (output_invalid): Use snprintf instead of
> 	sprintf.
> 	* config/tc-ia64.c (declare_register_set): Likewise.
> 	(emit_one_bundle): Likewise.
> 	(check_dependencies): Likewise.
> 	* config/tc-tic30.c (output_invalid): Likewise.

Assuming that you have tested the targets involved and that there were 
no regressions then this patch is approved.

Cheers
   Nick

Re: PATCH: Fix buffer overflow in gas

[H. J. Lu]

On Tue, May 02, 2006 at 10:47:27AM +0100, Nick Clifton wrote:
> Hi H. J.
> 
> >There are some potential buffer overflows in gas. 8byte isn't enough
> >to hold a negative byte. This patch fixes them. Also we should use
> >snprintf instead of sprintf.
> 
> Did you test this patch ?  if so, please could you say how.

You can put some none ascii char in assembly code.

> 
> >2006-05-01  H.J. Lu  <hongjiu.lu@intel.com>
> >
> >	* config/tc-i386.c (output_invalid_buf): Change size to 16.
> >	* config/tc-tic30.c (output_invalid_buf): Likewise.
> >
> >	* config/tc-i386.c (output_invalid): Use snprintf instead of
> >	sprintf.
> >	* config/tc-ia64.c (declare_register_set): Likewise.
> >	(emit_one_bundle): Likewise.
> >	(check_dependencies): Likewise.
> >	* config/tc-tic30.c (output_invalid): Likewise.
> 
> Assuming that you have tested the targets involved and that there were 
> no regressions then this patch is approved.

Done.


H.J.

Re: PATCH: Fix buffer overflow in gas

[H. J. Lu]

On Tue, May 02, 2006 at 06:31:30AM -0700, H. J. Lu wrote:
> On Tue, May 02, 2006 at 10:47:27AM +0100, Nick Clifton wrote:
> > Hi H. J.
> > 
> > >There are some potential buffer overflows in gas. 8byte isn't enough
> > >to hold a negative byte. This patch fixes them. Also we should use
> > >snprintf instead of sprintf.
> > 
> > Did you test this patch ?  if so, please could you say how.
> 
> You can put some none ascii char in assembly code.
> 
> > 
> > >2006-05-01  H.J. Lu  <hongjiu.lu@intel.com>
> > >
> > >	* config/tc-i386.c (output_invalid_buf): Change size to 16.
> > >	* config/tc-tic30.c (output_invalid_buf): Likewise.
> > >
> > >	* config/tc-i386.c (output_invalid): Use snprintf instead of
> > >	sprintf.
> > >	* config/tc-ia64.c (declare_register_set): Likewise.
> > >	(emit_one_bundle): Likewise.
> > >	(check_dependencies): Likewise.
> > >	* config/tc-tic30.c (output_invalid): Likewise.
> > 
> > Assuming that you have tested the targets involved and that there were 
> > no regressions then this patch is approved.
> 
> Done.
> 

Jan suggested we should cast int to unsigned char.

foo.s:20: Error: invalid character (0xd6) in mnemonic

is better than

foo.s:20: Error: invalid character (0xfffffffd6) in mnemonic

I will check it in.

H.J.
---
2006-05-02  H.J. Lu  <hongjiu.lu@intel.com>
	    Jan Beulich  <jbeulich@novell.com>

	* config/tc-i386.c (output_invalid_buf): Change size for
	unsigned char.
	* config/tc-tic30.c (output_invalid_buf): Likewise.

	* config/tc-i386.c (output_invalid): Cast none-ascii char to
	unsigned char.
	* config/tc-tic30.c (output_invalid): Likewise.

--- gas/config/tc-i386.c.buf	2006-05-02 06:50:00.000000000 -0700
+++ gas/config/tc-i386.c	2006-05-02 07:12:03.000000000 -0700
@@ -5251,7 +5251,7 @@ md_atof (type, litP, sizeP)
   return 0;
 }
 \f
-static char output_invalid_buf[16];
+static char output_invalid_buf[sizeof (unsigned char) * 2 + 6];
 
 static char *
 output_invalid (c)
@@ -5262,7 +5262,7 @@ output_invalid (c)
 	      "'%c'", c);
   else
     snprintf (output_invalid_buf, sizeof (output_invalid_buf),
-	      "(0x%x)", (unsigned) c);
+	      "(0x%x)", (unsigned char) c);
   return output_invalid_buf;
 }
 
--- gas/config/tc-tic30.c.buf	2006-05-02 06:50:00.000000000 -0700
+++ gas/config/tc-tic30.c	2006-05-02 07:12:11.000000000 -0700
@@ -273,7 +273,7 @@ struct tic30_insn
 struct tic30_insn insn;
 static int found_parallel_insn;
 
-static char output_invalid_buf[16];
+static char output_invalid_buf[sizeof (unsigned char) * 2 + 6];
 
 static char *
 output_invalid (char c)
@@ -283,7 +283,7 @@ output_invalid (char c)
 	      "'%c'", c);
   else
     snprintf (output_invalid_buf, sizeof (output_invalid_buf), 
-	      "(0x%x)", (unsigned) c);
+	      "(0x%x)", (unsigned char) c);
   return output_invalid_buf;
 }
 
2006-05-02  H.J. Lu  <hongjiu.lu@intel.com>

	* config/tc-i386.c (output_invalid_buf): Change size for
	unsigned char.
	* config/tc-tic30.c (output_invalid_buf): Likewise.

	* config/tc-i386.c (output_invalid): Cast none-ascii char to
	unsigned char.
	* config/tc-tic30.c (output_invalid): Likewise.

--- gas/config/tc-i386.c.buf	2006-05-02 06:50:00.000000000 -0700
+++ gas/config/tc-i386.c	2006-05-02 07:12:03.000000000 -0700
@@ -5251,7 +5251,7 @@ md_atof (type, litP, sizeP)
   return 0;
 }
 \f
-static char output_invalid_buf[16];
+static char output_invalid_buf[sizeof (unsigned char) * 2 + 6];
 
 static char *
 output_invalid (c)
@@ -5262,7 +5262,7 @@ output_invalid (c)
 	      "'%c'", c);
   else
     snprintf (output_invalid_buf, sizeof (output_invalid_buf),
-	      "(0x%x)", (unsigned) c);
+	      "(0x%x)", (unsigned char) c);
   return output_invalid_buf;
 }
 
--- gas/config/tc-tic30.c.buf	2006-05-02 06:50:00.000000000 -0700
+++ gas/config/tc-tic30.c	2006-05-02 07:12:11.000000000 -0700
@@ -273,7 +273,7 @@ struct tic30_insn
 struct tic30_insn insn;
 static int found_parallel_insn;
 
-static char output_invalid_buf[16];
+static char output_invalid_buf[sizeof (unsigned char) * 2 + 6];
 
 static char *
 output_invalid (char c)
@@ -283,7 +283,7 @@ output_invalid (char c)
 	      "'%c'", c);
   else
     snprintf (output_invalid_buf, sizeof (output_invalid_buf), 
-	      "(0x%x)", (unsigned) c);
+	      "(0x%x)", (unsigned char) c);
   return output_invalid_buf;
 }
 

Re: PATCH: Fix buffer overflow in gas

[Jan Beulich]

>+static char output_invalid_buf[sizeof (unsigned char) * 2 + 6];
 
I'm sorry to say that this, but regardless of actual width of 'char' or 'unsigned char' the above declaration still
always produces an 8-unit array. What you mean is making room for as many hex digits as an 'unsigned char' can be
converted to. Without knowing CHAR_BIT (or equivalent) I cannot see how you would be able to derive that. Unfortunately,
previous work on binutils has shown that one apparently shouldn't make assumptions about the availability of limits.h
(despite this having been a standard header for at least 15 years), and hence CHAR_BIT is not generally usable here.
Where needed, I (and apparently others) generally made the assumption that if limits.h isn't available, then the machine
is supposed to be a 8-bit-bytes one (and bad luck to those perhaps hypothetical machines that aren't and that don't
provide the header).

Jan

Re: PATCH: Fix buffer overflow in gas

[H. J. Lu]

On Tue, May 02, 2006 at 04:36:19PM +0200, Jan Beulich wrote:
> >+static char output_invalid_buf[sizeof (unsigned char) * 2 + 6];
>  
> I'm sorry to say that this, but regardless of actual width of 'char' or 'unsigned char' the above declaration still
> always produces an 8-unit array. What you mean is making room for as many hex digits as an 'unsigned char' can be
> converted to. Without knowing CHAR_BIT (or equivalent) I cannot see how you would be able to derive that. Unfortunately,
> previous work on binutils has shown that one apparently shouldn't make assumptions about the availability of limits.h
> (despite this having been a standard header for at least 15 years), and hence CHAR_BIT is not generally usable here.
> Where needed, I (and apparently others) generally made the assumption that if limits.h isn't available, then the machine
> is supposed to be a 8-bit-bytes one (and bad luck to those perhaps hypothetical machines that aren't and that don't
> provide the header).

I don't want to get too complicated here. We won't have buffer overflow
anymore no matter what CHAR_BIT is. Feel free to improve it.


H.J.

Re: PATCH: Fix buffer overflow in gas

[Andreas Schwab]

"H. J. Lu" <hjl@lucon.org> writes:

> I don't want to get too complicated here. We won't have buffer overflow
> anymore no matter what CHAR_BIT is.

If you have more than 8 bits in a char you need more than two characters
to represent it as a hex number.  On the other hand, whether we get to
process more than an octet at a time in this case remains to be seen.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."