[PATCH 0/2] mmc: core: null pointer dereference bug

[PATCH 0/2] mmc: core: null pointer dereference bug

[Tobin C. Harding]

Various functions take as parameter an optional pointer. Pointer
should be guarded with non-NULL check before dereferencing.

While fixing this bug it was found that the file contains multiple
functions doing variations on the same thing, sdio_readb(),
sdio_writeb(), sdio_readw(), sdio_writew() etc. Although the functions
have very similar logic the code is laid out in a variety of
ways. This makes it overly complicated to read. There is a already a
nice clean chunk of code, if we use this format for all instances then
we will have cleaned up the code, reduced the line count and lessened
the cognitive load required while reading.

Patch 01 adds non-NULL check before dereference of pointer.

Patch 02 cleans up the return code to be simple and uniform.

Code has not been tested. sdio_io.c with patches applied has been
checked with checkpatch, Sparse, and Smatch. Each patch has been
applied and built on x86_64 and PowerPC

Tobin C. Harding (2):
  mmc: core: guard dereference of optional parameter
  mmc: core: simplify return code

 drivers/mmc/core/sdio_io.c | 54 ++++++++++++++++++----------------------------
 1 file changed, 21 insertions(+), 33 deletions(-)

-- 
2.7.4

[PATCH 1/2] mmc: core: guard dereference of optional parameter

[Tobin C. Harding]

Various functions take as parameter an optional pointer. Pointer
should be guarded with non-NULL check before dereferencing.

Add non-NULL check before dereference of pointer.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/mmc/core/sdio_io.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 74195d7..3482314 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -373,7 +373,8 @@ u8 sdio_readb(struct sdio_func *func, unsigned int addr, int *err_ret)
 	u8 val;
 
 	if (!func) {
-		*err_ret = -EINVAL;
+		if (err_ret)
+			*err_ret = -EINVAL;
 		return 0xFF;
 	}
 
@@ -407,7 +408,8 @@ void sdio_writeb(struct sdio_func *func, u8 b, unsigned int addr, int *err_ret)
 	int ret;
 
 	if (!func) {
-		*err_ret = -EINVAL;
+		if (err_ret)
+			*err_ret = -EINVAL;
 		return;
 	}
 
@@ -635,7 +637,8 @@ unsigned char sdio_f0_readb(struct sdio_func *func, unsigned int addr,
 	unsigned char val;
 
 	if (!func) {
-		*err_ret = -EINVAL;
+		if (err_ret)
+			*err_ret = -EINVAL;
 		return 0xFF;
 	}
 
@@ -673,7 +676,8 @@ void sdio_f0_writeb(struct sdio_func *func, unsigned char b, unsigned int addr,
 	int ret;
 
 	if (!func) {
-		*err_ret = -EINVAL;
+		if (err_ret)
+			*err_ret = -EINVAL;
 		return;
 	}
 
-- 
2.7.4

[PATCH 2/2] mmc: core: simplify return code

[Tobin C. Harding]

File contains multiple functions doing variations on the same thing,
sdio_readb(), sdio_writeb()f, sdio_readw(), sdio_writew()
etc. Although the functions have very similar logic the code is laid
out in a variety of ways. This makes it overly complicated to
read. There is a already a nice clean chunk of code, if we use this
format for all instances then we will have cleaned up the code,
reduced the line count and lessened the cognitive load required while
reading. Less lines equals less bugs.

Pick the most simple and clear code flow and change all functions to
be the same.

Signed-off-by: Tobin C. Harding <me@tobin.cc>
---
 drivers/mmc/core/sdio_io.c | 42 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index 3482314..d40744b 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -378,15 +378,11 @@ u8 sdio_readb(struct sdio_func *func, unsigned int addr, int *err_ret)
 		return 0xFF;
 	}
 
-	if (err_ret)
-		*err_ret = 0;
-
 	ret = mmc_io_rw_direct(func->card, 0, func->num, addr, 0, &val);
-	if (ret) {
-		if (err_ret)
-			*err_ret = ret;
+	if (err_ret)
+		*err_ret = ret;
+	if (ret)
 		return 0xFF;
-	}
 
 	return val;
 }
@@ -443,7 +439,7 @@ u8 sdio_writeb_readb(struct sdio_func *func, u8 write_byte,
 	if (err_ret)
 		*err_ret = ret;
 	if (ret)
-		val = 0xff;
+		return 0xff;
 
 	return val;
 }
@@ -531,15 +527,11 @@ u16 sdio_readw(struct sdio_func *func, unsigned int addr, int *err_ret)
 {
 	int ret;
 
-	if (err_ret)
-		*err_ret = 0;
-
 	ret = sdio_memcpy_fromio(func, func->tmpbuf, addr, 2);
-	if (ret) {
-		if (err_ret)
-			*err_ret = ret;
+	if (err_ret)
+		*err_ret = ret;
+	if (ret)
 		return 0xFFFF;
-	}
 
 	return le16_to_cpup((__le16 *)func->tmpbuf);
 }
@@ -583,15 +575,11 @@ u32 sdio_readl(struct sdio_func *func, unsigned int addr, int *err_ret)
 {
 	int ret;
 
-	if (err_ret)
-		*err_ret = 0;
-
 	ret = sdio_memcpy_fromio(func, func->tmpbuf, addr, 4);
-	if (ret) {
-		if (err_ret)
-			*err_ret = ret;
+	if (err_ret)
+		*err_ret = ret;
+	if (ret)
 		return 0xFFFFFFFF;
-	}
 
 	return le32_to_cpup((__le32 *)func->tmpbuf);
 }
@@ -642,15 +630,11 @@ unsigned char sdio_f0_readb(struct sdio_func *func, unsigned int addr,
 		return 0xFF;
 	}
 
-	if (err_ret)
-		*err_ret = 0;
-
 	ret = mmc_io_rw_direct(func->card, 0, 0, addr, 0, &val);
-	if (ret) {
-		if (err_ret)
-			*err_ret = ret;
+	if (err_ret)
+		*err_ret = ret;
+	if (ret)
 		return 0xFF;
-	}
 
 	return val;
 }
-- 
2.7.4

Re: [PATCH 0/2] mmc: core: null pointer dereference bug

[Ulf Hansson]

On 15 March 2017 at 09:48, Tobin C. Harding <me@tobin.cc> wrote:
> Various functions take as parameter an optional pointer. Pointer
> should be guarded with non-NULL check before dereferencing.
>
> While fixing this bug it was found that the file contains multiple
> functions doing variations on the same thing, sdio_readb(),
> sdio_writeb(), sdio_readw(), sdio_writew() etc. Although the functions
> have very similar logic the code is laid out in a variety of
> ways. This makes it overly complicated to read. There is a already a
> nice clean chunk of code, if we use this format for all instances then
> we will have cleaned up the code, reduced the line count and lessened
> the cognitive load required while reading.
>
> Patch 01 adds non-NULL check before dereference of pointer.
>
> Patch 02 cleans up the return code to be simple and uniform.
>
> Code has not been tested. sdio_io.c with patches applied has been
> checked with checkpatch, Sparse, and Smatch. Each patch has been
> applied and built on x86_64 and PowerPC
>
> Tobin C. Harding (2):
>   mmc: core: guard dereference of optional parameter
>   mmc: core: simplify return code
>
>  drivers/mmc/core/sdio_io.c | 54 ++++++++++++++++++----------------------------
>  1 file changed, 21 insertions(+), 33 deletions(-)
>
> --
> 2.7.4
>

Thanks, applied for next!

Kind regards
Uffe