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

Groups > linux.kernel > #1697648 > unrolled thread

[PATCH v3/resubmit 1/3] staging: gs_fpgaboot: add buffer overflow checks

Back to article view | Back to linux.kernel

Contents

  [PATCH v3/resubmit 1/3] staging: gs_fpgaboot: add buffer overflow checks Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-27 03:20 +0200
    [PATCH v3/resubmit 3/3] staging: gs_fpgaboot: return valid error codes Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-27 03:20 +0200
    [PATCH v3/resubmit 2/3] staging: gs_fpgaboot: change char to u8 Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-27 03:20 +0200
    Re: [PATCH v3/resubmit 1/3] staging: gs_fpgaboot: add buffer  overflow checks Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-28 07:00 +0200

#1697648 — [PATCH v3/resubmit 1/3] 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.

Signed-off-by: Jacob von Chorus <jacobvonchorus@cwphoto.ca>

v3:
- use >= to prevent an integer overflow in the comparison
- use get_unaligned_be functions to interpret length fields
- fix remainder of file to use valid error codes

v2:
- char arrays converted to u8 arrays
- replace error return value with proper error code in
	gs_read_bitstream
---
 drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 54 +++++++++++++++++++++++--------
 1 file changed, 40 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
index 19b550fff0..a49019af60 100644
--- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
+++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
@@ -23,6 +23,7 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/firmware.h>
+#include <asm/unaligned.h>
 
 #include "gs_fpgaboot.h"
 #include "io.h"
@@ -47,7 +48,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 size, int *offset)
 {
 	char tbuf[64];
 	s32 len;
@@ -58,10 +59,16 @@ static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
 	/* read length */
 	read_bitstream(bitdata, tbuf, offset, 2);
 
-	len = tbuf[0] << 8 | tbuf[1];
+	len = get_unaligned_be16(tbuf);
+	if (len >= size) {
+		pr_err("error: readinfo buffer too small\n");
+		return -EINVAL;
+	}
 
 	read_bitstream(bitdata, buf, offset, len);
 	buf[len] = '\0';
+
+	return 0;
 }
 
 /*
@@ -83,8 +90,7 @@ static int readlength_bitstream(char *bitdata, int *lendata, int *offset)
 	/* read 4bytes length */
 	read_bitstream(bitdata, tbuf, offset, 4);
 
-	*lendata = tbuf[0] << 24 | tbuf[1] << 16 |
-		tbuf[2] << 8 | tbuf[3];
+	*lendata = get_unaligned_be32(tbuf);
 
 	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,34 +133,54 @@ 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;
+	int err;
 
 	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);
+	err = readmagic_bitstream(bitdata, &offset);
+	if (err)
+		return err;
+
+	err = readinfo_bitstream(bitdata, fimage->filename, MAX_STR, &offset);
+	if (err)
+		return err;
+	err = readinfo_bitstream(bitdata, fimage->part, MAX_STR, &offset);
+	if (err)
+		return err;
+	err = readinfo_bitstream(bitdata, fimage->date, MAX_STR, &offset);
+	if (err)
+		return err;
+	err = readinfo_bitstream(bitdata, fimage->time, MAX_STR, &offset);
+	if (err)
+		return err;
+
+	err = readlength_bitstream(bitdata, &fimage->lendata, &offset);
+	if (err)
+		return err;
 
 	fimage->fpgadata = bitdata + offset;
+
+	return 0;
 }
 
 static int gs_read_image(struct fpgaimage *fimage)
 {
 	int img_fmt;
+	int err;
 
-	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);
+		err = gs_read_bitstream(fimage);
+		if (err)
+			return err;
 		break;
 	default:
 		pr_err("unsupported fpga image format\n");
-- 
2.11.0

[toc] | [next] | [standalone]

#1697649 — [PATCH v3/resubmit 3/3] staging: gs_fpgaboot: return valid error codes

The return values on error are modified to be valid error codes. Theses
error codes are propagated back to the init function's return.

Signed-off-by: Jacob von Chorus <jacobvonchorus@cwphoto.ca>
---
 drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
index ff59708792..bcbdc7340b 100644
--- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
+++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
@@ -84,7 +84,7 @@ static int readlength_bitstream(u8 *bitdata, int *lendata, int *offset)
 	/* make sure it is section 'e' */
 	if (tbuf[0] != 'e') {
 		pr_err("error: length section is not 'e', but %c\n", tbuf[0]);
-		return -1;
+		return -EINVAL;
 	}
 
 	/* read 4bytes length */
@@ -107,7 +107,7 @@ static int readmagic_bitstream(u8 *bitdata, int *offset)
 	r = memcmp(buf, bits_magic, 13);
 	if (r) {
 		pr_err("error: corrupted header");
-		return -1;
+		return -EINVAL;
 	}
 	pr_info("bitstream file magic number Ok\n");
 
@@ -184,7 +184,7 @@ static int gs_read_image(struct fpgaimage *fimage)
 		break;
 	default:
 		pr_err("unsupported fpga image format\n");
-		return -1;
+		return -EINVAL;
 	}
 
 	gs_print_header(fimage);
@@ -223,7 +223,7 @@ static int gs_download_image(struct fpgaimage *fimage, enum wbus bus_bytes)
 	if (!xl_supported_prog_bus_width(bus_bytes)) {
 		pr_err("unsupported program bus width %d\n",
 		       bus_bytes);
-		return -1;
+		return -EINVAL;
 	}
 
 	/* Bring csi_b, rdwr_b Low and program_b High */
@@ -250,7 +250,7 @@ static int gs_download_image(struct fpgaimage *fimage, enum wbus bus_bytes)
 	/* Check INIT_B */
 	if (xl_get_init_b() == 0) {
 		pr_err("init_b 0\n");
-		return -1;
+		return -EIO;
 	}
 
 	while (xl_get_done_b() == 0) {
@@ -262,7 +262,7 @@ static int gs_download_image(struct fpgaimage *fimage, enum wbus bus_bytes)
 
 	if (cnt > MAX_WAIT_DONE) {
 		pr_err("fpga download fail\n");
-		return -1;
+		return -EIO;
 	}
 
 	pr_info("download fpgaimage\n");
@@ -351,7 +351,7 @@ static int gs_fpgaboot(void)
 err_out1:
 	kfree(fimage);
 
-	return -1;
+	return err;
 }
 
 static int __init gs_fpgaboot_init(void)
-- 
2.11.0

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

#1697650 — [PATCH v3/resubmit 2/3] staging: gs_fpgaboot: change char to u8

The bitstream storage variables were changed from char to u8 arrays to
prevent issues such as negative lengths. This change makes the code
compatible with the "data" field in "struct firmware" which is of type
u8.

Signed-off-by: Jacob von Chorus <jacobvonchorus@cwphoto.ca>

v3:
- reduce temporary buffer size in bitstream reading functions
---
 drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 24 ++++++++++++------------
 drivers/staging/gs_fpgaboot/gs_fpgaboot.h |  2 +-
 2 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
index a49019af60..ff59708792 100644
--- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
+++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
@@ -42,16 +42,16 @@ static char	*file = "xlinx_fpga_firmware.bit";
 module_param(file, charp, 0444);
 MODULE_PARM_DESC(file, "Xilinx FPGA firmware file.");
 
-static void read_bitstream(char *bitdata, char *buf, int *offset, int rdsize)
+static void read_bitstream(u8 *bitdata, u8 *buf, int *offset, int rdsize)
 {
 	memcpy(buf, bitdata + *offset, rdsize);
 	*offset += rdsize;
 }
 
-static int readinfo_bitstream(char *bitdata, char *buf, int size, int *offset)
+static int readinfo_bitstream(u8 *bitdata, u8 *buf, int size, int *offset)
 {
-	char tbuf[64];
-	s32 len;
+	u8 tbuf[2];
+	u16 len;
 
 	/* read section char */
 	read_bitstream(bitdata, tbuf, offset, 1);
@@ -74,9 +74,9 @@ static int readinfo_bitstream(char *bitdata, char *buf, int size, int *offset)
 /*
  * read bitdata length
  */
-static int readlength_bitstream(char *bitdata, int *lendata, int *offset)
+static int readlength_bitstream(u8 *bitdata, int *lendata, int *offset)
 {
-	char tbuf[64];
+	u8 tbuf[4];
 
 	/* read section char */
 	read_bitstream(bitdata, tbuf, offset, 1);
@@ -98,9 +98,9 @@ static int readlength_bitstream(char *bitdata, int *lendata, int *offset)
 /*
  * read first 13 bytes to check bitstream magic number
  */
-static int readmagic_bitstream(char *bitdata, int *offset)
+static int readmagic_bitstream(u8 *bitdata, int *offset)
 {
-	char buf[13];
+	u8 buf[13];
 	int r;
 
 	read_bitstream(bitdata, buf, offset, 13);
@@ -135,12 +135,12 @@ static void gs_print_header(struct fpgaimage *fimage)
 
 static int gs_read_bitstream(struct fpgaimage *fimage)
 {
-	char *bitdata;
+	u8 *bitdata;
 	int offset;
 	int err;
 
 	offset = 0;
-	bitdata = (char *)fimage->fw_entry->data;
+	bitdata = (u8 *)fimage->fw_entry->data;
 
 	err = readmagic_bitstream(bitdata, &offset);
 	if (err)
@@ -209,11 +209,11 @@ static int gs_load_image(struct fpgaimage *fimage, char *fw_file)
 
 static int gs_download_image(struct fpgaimage *fimage, enum wbus bus_bytes)
 {
-	char *bitdata;
+	u8 *bitdata;
 	int size, i, cnt;
 
 	cnt = 0;
-	bitdata = (char *)fimage->fpgadata;
+	bitdata = (u8 *)fimage->fpgadata;
 	size = fimage->lendata;
 
 #ifdef DEBUG_FPGA
diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.h b/drivers/staging/gs_fpgaboot/gs_fpgaboot.h
index cd1eb2c4c9..986e841f6b 100644
--- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.h
+++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.h
@@ -47,5 +47,5 @@ struct fpgaimage {
 	char	date[MAX_STR];
 	char	time[MAX_STR];
 	int	lendata;
-	char	*fpgadata;
+	u8	*fpgadata;
 };
-- 
2.11.0

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

#1698471 — Re: [PATCH v3/resubmit 1/3] staging: gs_fpgaboot: add buffer overflow checks

On Wed, Jul 26, 2017 at 09:13:57PM -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.
> 
> Signed-off-by: Jacob von Chorus <jacobvonchorus@cwphoto.ca>
> 
> v3:
> - use >= to prevent an integer overflow in the comparison
> - use get_unaligned_be functions to interpret length fields
> - fix remainder of file to use valid error codes
> 
> v2:
> - char arrays converted to u8 arrays
> - replace error return value with proper error code in
> 	gs_read_bitstream
> ---

All of the v2: and such needs to go below the --- line, as
Documentation/SubmittingPatches says to do.

Please fix that up and resend the series.

thanks,

greg k-h

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web