[PATCH v2 0/3] staging: fbtft: Fix buffer overflow vulnerability

[PATCH v2 0/3] staging: fbtft: Fix buffer overflow vulnerability

[Tobin C. Harding]

Module copies a user supplied string (module parameter) into a buffer
using strncpy() and does not check that the buffer is null terminated.

Replace call to strncpy() with call to strlcpy() ensuring that the
buffer is null terminated.

Replace magic number with pre-existing compile time constant.

Check return value of call to strlcpy() and throw warning if source
string is truncated.

v1 was a single patch. v2 adds 2 extra patches while retaining the
original v1 patch as the first of the series.

v2:
 - Replace magic number
 - Check return value of call to strlcpy()

Tobin C. Harding (3):
  staging: fbtft: Fix buffer overflow vulnerability
  staging: fbtft: Replace magic number with constant
  staging: fbtft: Add check on strlcpy() return value

 drivers/staging/fbtft/fbtft_device.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] staging: fbtft: Fix buffer overflow vulnerability

[Tobin C. Harding]

Module copies a user supplied string (module parameter) into a buffer
using strncpy() and does not check that the buffer is null terminated.

Replace call to strncpy() with call to strlcpy() ensuring that the
buffer is null terminated.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/fbtft/fbtft_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c b/drivers/staging/fbtft/fbtft_device.c
index de46f8d..7b7223b 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1483,7 +1483,7 @@ static int __init fbtft_device_init(void)
 			displays[i].pdev->name = name;
 			displays[i].spi = NULL;
 		} else {
-			strncpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
+			strlcpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
 			displays[i].pdev = NULL;
 		}
 	}
-- 
2.7.4

[PATCH v2 2/3] staging: fbtft: Replace magic number with constant

[Tobin C. Harding]

Current call to strncmp() uses a magic number. There is a compile
time constant defined for this buffer, included and used already at
other sites in the file.

Remove magic number. Replace with pre-existing compile time constant.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/fbtft/fbtft_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c b/drivers/staging/fbtft/fbtft_device.c
index 7b7223b..5fbdd37 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1489,7 +1489,7 @@ static int __init fbtft_device_init(void)
 	}
 
 	for (i = 0; i < ARRAY_SIZE(displays); i++) {
-		if (strncmp(name, displays[i].name, 32) == 0) {
+		if (strncmp(name, displays[i].name, SPI_NAME_SIZE) == 0) {
 			if (displays[i].spi) {
 				spi = displays[i].spi;
 				spi->chip_select = cs;
-- 
2.7.4

Re: [PATCH v2 2/3] staging: fbtft: Replace magic number with constant

[Joe Perches]

On Wed, 2017-02-15 at 14:27 +1100, Tobin C. Harding wrote:
> Current call to strncmp() uses a magic number. There is a compile
> time constant defined for this buffer, included and used already at
> other sites in the file.
> 
> Remove magic number. Replace with pre-existing compile time constant.

OK thanks, as well:

> diff --git a/drivers/staging/fbtft/fbtft_device.c b/drivers/staging/fbtft/fbtft_device.c
[]
> @@ -1489,7 +1489,7 @@ static int __init fbtft_device_init(void)
>  	}
>  
>  	for (i = 0; i < ARRAY_SIZE(displays); i++) {
> -		if (strncmp(name, displays[i].name, 32) == 0) {
> +		if (strncmp(name, displays[i].name, SPI_NAME_SIZE) == 0) {

Maybe change this to:

		if (strncmp(name, displays[i].name, SPI_NAME_SIZE) != 0)
			continue;

and reduce the indentation of the rest of the block.

[PATCH v2 3/3] staging: fbtft: Add check on strlcpy() return value

[Tobin C. Harding]

Return value of strlcpy() is not checked. Name string is silently
truncated if longer that SPI_NAME_SIZE, whilst not detrimental to
the program logic it would be nice to notify the user. Module is
currently quite verbose, adding extra pr_warn() calls will not overly
impact this verbosity.

Check return value from call to strlcpy(). If source string is
truncated call pr_warn() to notify user.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/staging/fbtft/fbtft_device.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/fbtft/fbtft_device.c b/drivers/staging/fbtft/fbtft_device.c
index 5fbdd37..78baf46 100644
--- a/drivers/staging/fbtft/fbtft_device.c
+++ b/drivers/staging/fbtft/fbtft_device.c
@@ -1483,7 +1483,13 @@ static int __init fbtft_device_init(void)
 			displays[i].pdev->name = name;
 			displays[i].spi = NULL;
 		} else {
-			strlcpy(displays[i].spi->modalias, name, SPI_NAME_SIZE);
+			size_t len;
+
+			len = strlcpy(displays[i].spi->modalias, name,
+				SPI_NAME_SIZE);
+			if (len >= SPI_NAME_SIZE)
+				pr_warn("modalias (name) truncated to: %s\n",
+					displays[i].spi->modalias);
 			displays[i].pdev = NULL;
 		}
 	}
-- 
2.7.4