Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1688577 > unrolled thread

[PATCH] staging: gs_fpgaboot: add buffer overflow checks

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: gs_fpgaboot: add buffer overflow checks Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-17 02:40 +0200
    Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-17 14:30 +0200
    Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-17 22:00 +0200
      Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-18 02:30 +0200
        Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-18 10:00 +0200

#1688577 — [PATCH] staging: gs_fpgaboot: add buffer overflow checks

Four fields in struct fpgaimage are char arrays of length MAX_STR (256).
The amount of data read into these buffers is controlled by a length
field in the bitstream file read from userspace. If a corrupt or
malicious firmware file was supplied, kernel data beyond these buffers
can be overwritten arbitrarily.

This patch adds a check of the bitstream's length value to ensure it
fits within the bounds of the allocated buffers. An error condition is
returned from gs_read_bitstream if any of the reads fail.

This patch also fixes a checkpatch.pl CHECK in io.c by removing the FSF
address paragraph.

Signed-off-by: Jacob von Chorus <jacobvonchorus@cwphoto.ca>
---
 drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 39 ++++++++++++++++++++++---------
 drivers/staging/gs_fpgaboot/io.c          |  4 ----
 2 files changed, 28 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
index 19b550fff0..2aafd769b8 100644
--- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
+++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
@@ -47,7 +47,7 @@ static void read_bitstream(char *bitdata, char *buf, int *offset, int rdsize)
 	*offset += rdsize;
 }
 
-static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
+static int readinfo_bitstream(char *bitdata, char *buf, int n, int *offset)
 {
 	char tbuf[64];
 	s32 len;
@@ -59,9 +59,15 @@ static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
 	read_bitstream(bitdata, tbuf, offset, 2);
 
 	len = tbuf[0] << 8 | tbuf[1];
+	if (len + 1 > n) {
+		pr_err("error: readinfo buffer too small\n");
+		return -1;
+	}
 
 	read_bitstream(bitdata, buf, offset, len);
 	buf[len] = '\0';
+
+	return 0;
 }
 
 /*
@@ -113,7 +119,7 @@ static int readmagic_bitstream(char *bitdata, int *offset)
 /*
  * NOTE: supports only bitstream format
  */
-static enum fmt_image get_imageformat(struct fpgaimage *fimage)
+static enum fmt_image get_imageformat(void)
 {
 	return f_bit;
 }
@@ -127,7 +133,7 @@ static void gs_print_header(struct fpgaimage *fimage)
 	pr_info("lendata: %d\n", fimage->lendata);
 }
 
-static void gs_read_bitstream(struct fpgaimage *fimage)
+static int gs_read_bitstream(struct fpgaimage *fimage)
 {
 	char *bitdata;
 	int offset;
@@ -135,26 +141,37 @@ static void gs_read_bitstream(struct fpgaimage *fimage)
 	offset = 0;
 	bitdata = (char *)fimage->fw_entry->data;
 
-	readmagic_bitstream(bitdata, &offset);
-	readinfo_bitstream(bitdata, fimage->filename, &offset);
-	readinfo_bitstream(bitdata, fimage->part, &offset);
-	readinfo_bitstream(bitdata, fimage->date, &offset);
-	readinfo_bitstream(bitdata, fimage->time, &offset);
-	readlength_bitstream(bitdata, &fimage->lendata, &offset);
+	if (readmagic_bitstream(bitdata, &offset))
+		return -1;
+
+	if (readinfo_bitstream(bitdata, fimage->filename, MAX_STR, &offset))
+		return -1;
+	if (readinfo_bitstream(bitdata, fimage->part, MAX_STR, &offset))
+		return -1;
+	if (readinfo_bitstream(bitdata, fimage->date, MAX_STR, &offset))
+		return -1;
+	if (readinfo_bitstream(bitdata, fimage->time, MAX_STR, &offset))
+		return -1;
+
+	if (readlength_bitstream(bitdata, &fimage->lendata, &offset))
+		return -1;
 
 	fimage->fpgadata = bitdata + offset;
+
+	return 0;
 }
 
 static int gs_read_image(struct fpgaimage *fimage)
 {
 	int img_fmt;
 
-	img_fmt = get_imageformat(fimage);
+	img_fmt = get_imageformat();
 
 	switch (img_fmt) {
 	case f_bit:
 		pr_info("image is bitstream format\n");
-		gs_read_bitstream(fimage);
+		if (gs_read_bitstream(fimage))
+			return -1;
 		break;
 	default:
 		pr_err("unsupported fpga image format\n");
diff --git a/drivers/staging/gs_fpgaboot/io.c b/drivers/staging/gs_fpgaboot/io.c
index c9391198fb..83a13ca725 100644
--- a/drivers/staging/gs_fpgaboot/io.c
+++ b/drivers/staging/gs_fpgaboot/io.c
@@ -9,10 +9,6 @@
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  *
  * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program; if not, write to the Free Software
- * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
  */
 
 #include <linux/kernel.h>
-- 
2.11.0

[toc] | [next] | [standalone]

#1688983

On Sun, Jul 16, 2017 at 08:38:41PM -0400, Jacob von Chorus wrote:
> Four fields in struct fpgaimage are char arrays of length MAX_STR (256).
> The amount of data read into these buffers is controlled by a length
> field in the bitstream file read from userspace. If a corrupt or
> malicious firmware file was supplied, kernel data beyond these buffers
> can be overwritten arbitrarily.
> 
> This patch adds a check of the bitstream's length value to ensure it
> fits within the bounds of the allocated buffers. An error condition is
> returned from gs_read_bitstream if any of the reads fail.
> 
> This patch also fixes a checkpatch.pl CHECK in io.c by removing the FSF
> address paragraph.

Whenever you have a "also" in a patch changelog, that's a huge flag that
this should be a separate patch.  As is the case here, fixing the FSF
address has nothing to do with the buffer overflow checks.

Please break this up into two different patches.


thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1689416

On Sun, Jul 16, 2017 at 08:38:41PM -0400, Jacob von Chorus wrote:
> diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> index 19b550fff0..2aafd769b8 100644
> --- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> +++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> @@ -47,7 +47,7 @@ static void read_bitstream(char *bitdata, char *buf, int *offset, int rdsize)
>  	*offset += rdsize;
>  }
>  
> -static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
> +static int readinfo_bitstream(char *bitdata, char *buf, int n, int *offset)

Choose a better name than "n" like "size".

>  {
>  	char tbuf[64];
>  	s32 len;
> @@ -59,9 +59,15 @@ static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
>  	read_bitstream(bitdata, tbuf, offset, 2);
>  
>  	len = tbuf[0] << 8 | tbuf[1];

Since tbuf is char then, on x86 and arm, that means it's signed and it
means "len" can be negative.  Declare tbuf as u8.  Which will require
other changes as well...

> +	if (len + 1 > n) {

It's more idiomatic to say "if (len >= n)".  Plus that's a good habbit
if you want to avoid integer overflows.

> +		pr_err("error: readinfo buffer too small\n");
> +		return -1;

-1 is not a correct error code.

regards,
dan carpenter

[toc] | [prev] | [next] | [standalone]

#1689578

On Mon, Jul 17, 2017 at 10:53:25PM +0300, Dan Carpenter wrote:
> > +	if (len + 1 > n) {
> 
> It's more idiomatic to say "if (len >= n)".  Plus that's a good habbit

My reasoning behind using "((len + 1) > n)" is that len represents the length of
the string without null-termination. "buf" is required to store a
null-terminator on top of len. Using "len + 1" shows this requirement
more clearly; I will add brackets around "len + 1" for emphasis.

Thanks for the feedback, I will send a v2.

Regards,
Jacob von Chorus

[toc] | [prev] | [next] | [standalone]

#1689845

On Mon, Jul 17, 2017 at 08:21:20PM -0400, Jacob von Chorus wrote:
> On Mon, Jul 17, 2017 at 10:53:25PM +0300, Dan Carpenter wrote:
> > > +	if (len + 1 > n) {
> > 
> > It's more idiomatic to say "if (len >= n)".  Plus that's a good habbit
> 
> My reasoning behind using "((len + 1) > n)" is that len represents the length of
> the string without null-termination. "buf" is required to store a
> null-terminator on top of len. Using "len + 1" shows this requirement
> more clearly; I will add brackets around "len + 1" for emphasis.
> 

Don't get into the habbit of saying len + 1 because you will end up
introducing integer overflows.  Also don't add useless parenthesis.
Everyone who programs in C is used to NUL terminators, so it's not a new
concept which has to be explained.

regards,
dan carpenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web