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

Groups > linux.kernel > #1689580 > unrolled thread

[PATCH v2 1/2] staging: gs_fpgaboot: add buffer overflow checks

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/2] staging: gs_fpgaboot: add buffer overflow checks Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-18 02:50 +0200
    [PATCH v2 2/2] staging: gs_fpgaboot: change char to u8 Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-18 02:50 +0200
      Re: [PATCH v2 2/2] staging: gs_fpgaboot: change char to u8 Joe Perches <joe@perches.com> - 2017-07-18 03:30 +0200
        Re: [PATCH v2 2/2] staging: gs_fpgaboot: change char to u8 Jacob von Chorus <jacobvonchorus@cwphoto.ca> - 2017-07-18 04:00 +0200
    Re: [PATCH v2 1/2] staging: gs_fpgaboot: add buffer overflow checks Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-07-18 09:10 +0200
    Re: [PATCH v2 1/2] staging: gs_fpgaboot: add buffer overflow checks Dan Carpenter <dan.carpenter@oracle.com> - 2017-07-18 11:20 +0200

#1689580 — [PATCH v2 1/2] 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>
---
 drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 48 ++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
index 19b550fff0..008ef99f05 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 size, 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) > size) {
+		pr_err("error: readinfo buffer too small\n");
+		return -ETOOSMALL;
+	}
 
 	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,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]

#1689584 — [PATCH v2 2/2] 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>
---
 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 008ef99f05..467a0ad81f 100644
--- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
+++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
@@ -41,16 +41,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[64];
+	u16 len;
 
 	/* read section char */
 	read_bitstream(bitdata, tbuf, offset, 1);
@@ -73,9 +73,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[64];
 
 	/* 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]

#1689593 — Re: [PATCH v2 2/2] staging: gs_fpgaboot: change char to u8

On Mon, 2017-07-17 at 20:47 -0400, Jacob von Chorus wrote:
> 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>
> ---
>  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 008ef99f05..467a0ad81f 100644
> --- a/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> +++ b/drivers/staging/gs_fpgaboot/gs_fpgaboot.c
> @@ -41,16 +41,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[64];
> +	u16 len;
>  
>  	/* read section char */
>  	read_bitstream(bitdata, tbuf, offset, 1);

read_bitstream takes an int rdsize, not a u16.
and this function will overflow tbuf if len > 64

static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
{
	char tbuf[64];
	s32 len;

	/* read section char */
	read_bitstream(bitdata, tbuf, offset, 1);

	/* read length */
	read_bitstream(bitdata, tbuf, offset, 2);

	len = tbuf[0] << 8 | tbuf[1];

	read_bitstream(bitdata, buf, offset, len);
	buf[len] = '\0';
}

len is up to 64k but tbuf is 64 bytes.

	len = get_unaligned_le16(tbuf)

might be nicer than

	len = tbuf[0] << 8 | tbuf[1];

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

#1689608 — Re: [PATCH v2 2/2] staging: gs_fpgaboot: change char to u8

On Mon, Jul 17, 2017 at 06:22:08PM -0700, Joe Perches wrote:
> read_bitstream takes an int rdsize, not a u16.
> and this function will overflow tbuf if len > 64
>
> static void readinfo_bitstream(char *bitdata, char *buf, int *offset)
> {
> 	char tbuf[64];
> 	s32 len;
> 
> 	/* read section char */
> 	read_bitstream(bitdata, tbuf, offset, 1);
> 
> 	/* read length */
> 	read_bitstream(bitdata, tbuf, offset, 2);
> 
> 	len = tbuf[0] << 8 | tbuf[1];
> 
> 	read_bitstream(bitdata, buf, offset, len);
> 	buf[len] = '\0';
> }
> 
> len is up to 64k but tbuf is 64 bytes.

tbuf is used here to read a total of 3 bytes over two calls to
read_bitstream. The larger read of size, len, is stored to buf which is
MAX_STR bytes in length. 

> 	len = get_unaligned_le16(tbuf)
> 
> might be nicer than
> 
> 	len = tbuf[0] << 8 | tbuf[1];

Agreed, though it should be "get_unaligned_be16". 

Thanks.

Regards,
Jacob von Chorus

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

#1689792

On Mon, Jul 17, 2017 at 08:47:25PM -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>
> ---
>  drivers/staging/gs_fpgaboot/gs_fpgaboot.c | 48 ++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 11 deletions(-)

What changed from v1?  Always list that below the --- line.

Please fix up and resend...

thanks,

greg k-h

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

#1689914

On Mon, Jul 17, 2017 at 08:47:25PM -0400, Jacob von Chorus wrote:
> -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;
> @@ -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) > size) {
> +		pr_err("error: readinfo buffer too small\n");
> +		return -ETOOSMALL;

Probably the correct error code is -EINVAL.  I should have just said
that in the first email.

regards,
dan carpenter

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web