[PATCH] staging: gs_fpgaboot: add buffer overflow checks

[PATCH] staging: gs_fpgaboot: add buffer overflow checks

[Jacob von Chorus]

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

Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks

[Greg Kroah-Hartman]

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

Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks

[Dan Carpenter]

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

[PATCH] staging: gs_fpgaboot: add buffer overflow checks

[Jacob von Chorus]

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 | 39 ++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 11 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");
-- 
2.11.0

Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks

[Jacob von Chorus]

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

Re: [PATCH] staging: gs_fpgaboot: add buffer overflow checks

[Dan Carpenter]

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