[PATCH] mtd: gpmi: Fix NULL pointer dereference

[PATCH] mtd: gpmi: Fix NULL pointer dereference

[Fabio Estevam]

From: Fabio Estevam <fabio.estevam@freescale.com>

Currently mx23_check_transcription_stamp() uses chip->buffers->databuf
as its buffer, which is allocated by nand_scan_tail().

Since commit 720e7ce5 ("mtd: gpmi: remove the nand_scan()"), 
mx23_check_transcription_stamp() is called before nand_scan_tail(), which causes
a NULL pointer dereference:

[    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
[    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
[    1.170000] pgd = c0004000
[    1.170000] [000005d0] *pgd=00000000
[    1.180000] Internal error: Oops: 5 [#1] ARM
[    1.180000] Modules linked in:
[    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
[    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
[    1.180000] PC is at memcmp+0x10/0x54
[    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
[    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
[    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
[    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
[    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
[    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
[    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
[    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
[    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)

In order to fix this problem, allocate the buffer locally via kzalloc().

Also, as mx23_check_transcription_stamp() can return en error code now, adapt
the logic in mx23_boot_init() to take this into account.

Cc: <stable@vger.kernel.org> # 3.12
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index a9830ff..e090280 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1342,7 +1342,7 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
 	unsigned int search_area_size_in_strides;
 	unsigned int stride;
 	unsigned int page;
-	uint8_t *buffer = chip->buffers->databuf;
+	uint8_t *buffer;
 	int saved_chip_number;
 	int found_an_ncb_fingerprint = false;
 
@@ -1352,6 +1352,9 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
 	saved_chip_number = this->current_chip;
 	chip->select_chip(mtd, 0);
 
+	buffer = kzalloc(strlen(fingerprint) * sizeof(*buffer), GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
 	/*
 	 * Loop through the first search area, looking for the NCB fingerprint.
 	 */
@@ -1380,6 +1383,8 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
 
 	chip->select_chip(mtd, saved_chip_number);
 
+	kfree(buffer);
+
 	if (found_an_ncb_fingerprint)
 		dev_dbg(dev, "\tFound a fingerprint\n");
 	else
@@ -1488,7 +1493,7 @@ static int mx23_boot_init(struct gpmi_nand_data  *this)
 	 * transcription stamp. If we find it, then we don't have to do
 	 * anything -- the block marks are already transcribed.
 	 */
-	if (mx23_check_transcription_stamp(this))
+	if (mx23_check_transcription_stamp(this) > 0)
 		return 0;
 
 	/*
-- 
1.8.1.2

Re: [PATCH] mtd: gpmi: Fix NULL pointer dereference

[Brian Norris]

Hi Fabio, Huang,

On Mon, Nov 11, 2013 at 03:08:47PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@freescale.com>
> 
> Currently mx23_check_transcription_stamp() uses chip->buffers->databuf
> as its buffer, which is allocated by nand_scan_tail().
> 
> Since commit 720e7ce5 ("mtd: gpmi: remove the nand_scan()"), 
> mx23_check_transcription_stamp() is called before nand_scan_tail(), which causes
> a NULL pointer dereference:
> 
> [    1.150000] NAND device: Manufacturer ID: 0xec, Chip ID: 0xd7 (Samsung NAND 4GiB 3,3V 8-bit), 4096MiB, page size: 4096, OOB size: 8
> [    1.160000] Unable to handle kernel NULL pointer dereference at virtual address 000005d0
> [    1.170000] pgd = c0004000
> [    1.170000] [000005d0] *pgd=00000000
> [    1.180000] Internal error: Oops: 5 [#1] ARM
> [    1.180000] Modules linked in:
> [    1.180000] CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0 #89
> [    1.180000] task: c7440000 ti: c743a000 task.ti: c743a000
> [    1.180000] PC is at memcmp+0x10/0x54
> [    1.180000] LR is at gpmi_nand_probe+0x42c/0x894
> [    1.180000] pc : [<c025fcb0>]    lr : [<c02f6a68>]    psr: 20000053
> [    1.180000] sp : c743be2c  ip : 600000d3  fp : ffffffff
> [    1.180000] r10: 000005d0  r9 : c02f5f08  r8 : 00000000
> [    1.180000] r7 : c75858a8  r6 : c75858a8  r5 : c7585b18  r4 : c7585800
> [    1.180000] r3 : 000005d0  r2 : 00000004  r1 : c05c33e4  r0 : 000005d0
> [    1.180000] Flags: nzCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> [    1.180000] Control: 0005317f  Table: 40004000  DAC: 00000017
> [    1.180000] Process swapper (pid: 1, stack limit = 0xc743a1c0)
> 
> In order to fix this problem, allocate the buffer locally via kzalloc().
> 
> Also, as mx23_check_transcription_stamp() can return en error code now, adapt
> the logic in mx23_boot_init() to take this into account.
> 
> Cc: <stable@vger.kernel.org> # 3.12
> Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>

OK, so there seem to be 3 forms of this patch now.

(1) The first used stack memory for potential DMA transactions. This is
    wrong. (Good catch, Huang.)

(2) The second switched the whole NAND buffer over to a kzalloc'd
    buffer, with NAND_OWN_BUFFERS. I'm not a huge fan of
    NAND_OWN_BUFFERS, and I think drivers should only be using it if
    they really need it, for special buffer placement or some other good
    reason. You just happen to need a buffer "too early", where you can
    really just allocate a small temporary buffer.

(3) This third form (the patch I'm replying to) just temporarily uses a
    kzalloc'd buffer for the fingerprint transactions, without exposing
    this buffer for use anywhere else.

I think fix 3 has the proper middle ground, especially for a -stable
fix, where we want to change as little as necessary. Can we get a
Tested-by or Acked-by from you, Huang? I'd like to get this fix in soon.

> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index a9830ff..e090280 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1342,7 +1342,7 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>  	unsigned int search_area_size_in_strides;
>  	unsigned int stride;
>  	unsigned int page;
> -	uint8_t *buffer = chip->buffers->databuf;
> +	uint8_t *buffer;
>  	int saved_chip_number;
>  	int found_an_ncb_fingerprint = false;
>  
> @@ -1352,6 +1352,9 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>  	saved_chip_number = this->current_chip;
>  	chip->select_chip(mtd, 0);
>  
> +	buffer = kzalloc(strlen(fingerprint) * sizeof(*buffer), GFP_KERNEL);

I think you might want to drop '* sizeof(*buffer)'. We really just want
strlen(fingerprint), no matter what data type we're using for the
'buffer' pointer.

> +	if (!buffer)
> +		return -ENOMEM;
>  	/*
>  	 * Loop through the first search area, looking for the NCB fingerprint.
>  	 */
> @@ -1380,6 +1383,8 @@ static int mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>  
>  	chip->select_chip(mtd, saved_chip_number);
>  
> +	kfree(buffer);
> +
>  	if (found_an_ncb_fingerprint)
>  		dev_dbg(dev, "\tFound a fingerprint\n");
>  	else
> @@ -1488,7 +1493,7 @@ static int mx23_boot_init(struct gpmi_nand_data  *this)
>  	 * transcription stamp. If we find it, then we don't have to do
>  	 * anything -- the block marks are already transcribed.
>  	 */
> -	if (mx23_check_transcription_stamp(this))
> +	if (mx23_check_transcription_stamp(this) > 0)
>  		return 0;

You might want to capture and return the negative error code path,
rather than ignoring it. e.g.:

	ret = mx23_check_transcription_stamp(this);
	if (ret < 0)
		return ret;
	else if (ret)
  		return 0;

>  
>  	/*

Brian

Re: [PATCH] mtd: gpmi: Fix NULL pointer dereference

[Huang Shijie]

于 2013年11月12日 02:23, Brian Norris 写道:
> (2) The second switched the whole NAND buffer over to a kzalloc'd
>     buffer, with NAND_OWN_BUFFERS. I'm not a huge fan of
>     NAND_OWN_BUFFERS, and I think drivers should only be using it if
>     they really need it, for special buffer placement or some other good
>     reason. You just happen to need a buffer "too early", where you can
>     really just allocate a small temporary buffer.
why I use the NAND_OWN_BUFFERS? just for a small temporary buffer?
the answer is no.

For imx23, the work flow is like this:
[1] first check the fingerprint, if we can find it, we will return
immediately.
[2] if [1] failed, such as you erase all the partitions, the gpmi will call
mx23_write_transcription_stamp() to write the fingerprint.

So the @chip->buffer is not only used by the
mx23_check_transcription_stamp(),
but _also_ used by the mx23_write_transcription_stamp() when the gpmi
can not find any
fingerprint in the NAND page.


That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both
the mx23_check_transcription_stamp()
and mx23_write_transcription_stamp().

thanks
Huang Shijie

Re: [PATCH] mtd: gpmi: Fix NULL pointer dereference

[Brian Norris]

On Mon, Nov 11, 2013 at 6:47 PM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年11月12日 02:23, Brian Norris 写道:
>> (2) The second switched the whole NAND buffer over to a kzalloc'd
>>     buffer, with NAND_OWN_BUFFERS. I'm not a huge fan of
>>     NAND_OWN_BUFFERS, and I think drivers should only be using it if
>>     they really need it, for special buffer placement or some other good
>>     reason. You just happen to need a buffer "too early", where you can
>>     really just allocate a small temporary buffer.
> why I use the NAND_OWN_BUFFERS? just for a small temporary buffer?
> the answer is no.
>
> For imx23, the work flow is like this:
> [1] first check the fingerprint, if we can find it, we will return
> immediately.
> [2] if [1] failed, such as you erase all the partitions, the gpmi will call
> mx23_write_transcription_stamp() to write the fingerprint.
>
> So the @chip->buffer is not only used by the
> mx23_check_transcription_stamp(),
> but _also_ used by the mx23_write_transcription_stamp() when the gpmi
> can not find any
> fingerprint in the NAND page.

Right, thanks for pointing this out.

> That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both
> the mx23_check_transcription_stamp()
> and mx23_write_transcription_stamp().

I will resume discussion on your NAND_OWN_BUFFERS patch.

Brian

Re: [PATCH] mtd: gpmi: Fix NULL pointer dereference

[Fabio Estevam]

On Tue, Nov 12, 2013 at 12:47 AM, Huang Shijie <b32955@freescale.com> wrote:
> 于 2013年11月12日 02:23, Brian Norris 写道:

> For imx23, the work flow is like this:
> [1] first check the fingerprint, if we can find it, we will return
> immediately.
> [2] if [1] failed, such as you erase all the partitions, the gpmi will call
> mx23_write_transcription_stamp() to write the fingerprint.
>
> So the @chip->buffer is not only used by the
> mx23_check_transcription_stamp(),
> but _also_ used by the mx23_write_transcription_stamp() when the gpmi
> can not find any
> fingerprint in the NAND page.
>
>
> That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both
> the mx23_check_transcription_stamp()
> and mx23_write_transcription_stamp().

Understood.

What if we just allocate the 4-byte buffer once on probe?

Like this:

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index a9830ff..647da1b 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1342,7 +1342,6 @@ static int mx23_check_transcription_stamp(struct
gpmi_nand_data *this)
     unsigned int search_area_size_in_strides;
     unsigned int stride;
     unsigned int page;
-    uint8_t *buffer = chip->buffers->databuf;
     int saved_chip_number;
     int found_an_ncb_fingerprint = false;

@@ -1368,10 +1367,10 @@ static int
mx23_check_transcription_stamp(struct gpmi_nand_data *this)
          * and starts in the 12th byte of the page.
          */
         chip->cmdfunc(mtd, NAND_CMD_READ0, 12, page);
-        chip->read_buf(mtd, buffer, strlen(fingerprint));
+        chip->read_buf(mtd, this->buffer, strlen(fingerprint));

         /* Look for the fingerprint. */
-        if (!memcmp(buffer, fingerprint, strlen(fingerprint))) {
+        if (!memcmp(this->buffer, fingerprint, strlen(fingerprint))) {
             found_an_ncb_fingerprint = true;
             break;
         }
@@ -1401,7 +1400,6 @@ static int mx23_write_transcription_stamp(struct
gpmi_nand_data *this)
     unsigned int block;
     unsigned int stride;
     unsigned int page;
-    uint8_t      *buffer = chip->buffers->databuf;
     int saved_chip_number;
     int status;

@@ -1442,9 +1440,9 @@ static int mx23_write_transcription_stamp(struct
gpmi_nand_data *this)
     }

     /* Write the NCB fingerprint into the page buffer. */
-    memset(buffer, ~0, mtd->writesize);
+    memset(this->buffer, ~0, mtd->writesize);
     memset(chip->oob_poi, ~0, mtd->oobsize);
-    memcpy(buffer + 12, fingerprint, strlen(fingerprint));
+    memcpy(this->buffer + 12, fingerprint, strlen(fingerprint));

     /* Loop through the first search area, writing NCB fingerprints. */
     dev_dbg(dev, "Writing NCB fingerprints...\n");
@@ -1455,7 +1453,7 @@ static int mx23_write_transcription_stamp(struct
gpmi_nand_data *this)
         /* Write the first page of the current stride. */
         dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page);
         chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
-        chip->ecc.write_page_raw(mtd, chip, buffer, 0);
+        chip->ecc.write_page_raw(mtd, chip, this->buffer, 0);
         chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);

         /* Wait for the write to finish. */
@@ -1728,6 +1726,10 @@ static int gpmi_nand_probe(struct platform_device *pdev)
         return -ENOMEM;
     }

+    this->buffer = devm_kzalloc(&pdev->dev, strlen(fingerprint), GFP_KERNEL);
+    if (!this->buffer)
+        return -ENOMEM;
+
     platform_set_drvdata(pdev, this);
     this->pdev  = pdev;
     this->dev   = &pdev->dev;
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index a7685e3..e88114c 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -186,6 +186,7 @@ struct gpmi_nand_data {

     /* private */
     void            *private;
+    uint8_t         *buffer;
 };

Re: [PATCH] mtd: gpmi: Fix NULL pointer dereference

[Huang Shijie]

于 2013年11月12日 11:44, Fabio Estevam 写道:
> On Tue, Nov 12, 2013 at 12:47 AM, Huang Shijie<b32955@freescale.com>  wrote:
>> 于 2013年11月12日 02:23, Brian Norris 写道:
>> For imx23, the work flow is like this:
>> [1] first check the fingerprint, if we can find it, we will return
>> immediately.
>> [2] if [1] failed, such as you erase all the partitions, the gpmi will call
>> mx23_write_transcription_stamp() to write the fingerprint.
>>
>> So the @chip->buffer is not only used by the
>> mx23_check_transcription_stamp(),
>> but _also_ used by the mx23_write_transcription_stamp() when the gpmi
>> can not find any
>> fingerprint in the NAND page.
>>
>>
>> That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both
>> the mx23_check_transcription_stamp()
>> and mx23_write_transcription_stamp().
> Understood.
>
> What if we just allocate the 4-byte buffer once on probe?
>
> Like this:
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index a9830ff..647da1b 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1342,7 +1342,6 @@ static int mx23_check_transcription_stamp(struct
> gpmi_nand_data *this)
>       unsigned int search_area_size_in_strides;
>       unsigned int stride;
>       unsigned int page;
> -    uint8_t *buffer = chip->buffers->databuf;
>       int saved_chip_number;
>       int found_an_ncb_fingerprint = false;
>
> @@ -1368,10 +1367,10 @@ static int
> mx23_check_transcription_stamp(struct gpmi_nand_data *this)
>            * and starts in the 12th byte of the page.
>            */
>           chip->cmdfunc(mtd, NAND_CMD_READ0, 12, page);
> -        chip->read_buf(mtd, buffer, strlen(fingerprint));
> +        chip->read_buf(mtd, this->buffer, strlen(fingerprint));
>
>           /* Look for the fingerprint. */
> -        if (!memcmp(buffer, fingerprint, strlen(fingerprint))) {
> +        if (!memcmp(this->buffer, fingerprint, strlen(fingerprint))) {
>               found_an_ncb_fingerprint = true;
>               break;
>           }
> @@ -1401,7 +1400,6 @@ static int mx23_write_transcription_stamp(struct
> gpmi_nand_data *this)
>       unsigned int block;
>       unsigned int stride;
>       unsigned int page;
> -    uint8_t      *buffer = chip->buffers->databuf;
>       int saved_chip_number;
>       int status;
>
> @@ -1442,9 +1440,9 @@ static int mx23_write_transcription_stamp(struct
> gpmi_nand_data *this)
>       }
>
>       /* Write the NCB fingerprint into the page buffer. */
> -    memset(buffer, ~0, mtd->writesize);
> +    memset(this->buffer, ~0, mtd->writesize);
>       memset(chip->oob_poi, ~0, mtd->oobsize);
NULL pointer here, since chip->oob_poi is NULL.
> -    memcpy(buffer + 12, fingerprint, strlen(fingerprint));
> +    memcpy(this->buffer + 12, fingerprint, strlen(fingerprint));
>
>       /* Loop through the first search area, writing NCB fingerprints. */
>       dev_dbg(dev, "Writing NCB fingerprints...\n");
> @@ -1455,7 +1453,7 @@ static int mx23_write_transcription_stamp(struct
> gpmi_nand_data *this)
>           /* Write the first page of the current stride. */
>           dev_dbg(dev, "Writing an NCB fingerprint in page 0x%x\n", page);
>           chip->cmdfunc(mtd, NAND_CMD_SEQIN, 0x00, page);
> -        chip->ecc.write_page_raw(mtd, chip, buffer, 0);
> +        chip->ecc.write_page_raw(mtd, chip, this->buffer, 0);
NULL pointer here.

thanks
Huang Shijie

Re: [PATCH] mtd: gpmi: Fix NULL pointer dereference

[Brian Norris]

On Mon, Nov 11, 2013 at 7:44 PM, Fabio Estevam <festevam@gmail.com> wrote:
> On Tue, Nov 12, 2013 at 12:47 AM, Huang Shijie <b32955@freescale.com> wrote:
>> 于 2013年11月12日 02:23, Brian Norris 写道:
>
>> For imx23, the work flow is like this:
>> [1] first check the fingerprint, if we can find it, we will return
>> immediately.
>> [2] if [1] failed, such as you erase all the partitions, the gpmi will call
>> mx23_write_transcription_stamp() to write the fingerprint.
>>
>> So the @chip->buffer is not only used by the
>> mx23_check_transcription_stamp(),
>> but _also_ used by the mx23_write_transcription_stamp() when the gpmi
>> can not find any
>> fingerprint in the NAND page.
>>
>>
>> That's why i use the NAND_OWN_BUFFERS, the buffer can be used by both
>> the mx23_check_transcription_stamp()
>> and mx23_write_transcription_stamp().
>
> Understood.
>
> What if we just allocate the 4-byte buffer once on probe?
>
> Like this:
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index a9830ff..647da1b 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
...
> @@ -1728,6 +1726,10 @@ static int gpmi_nand_probe(struct platform_device *pdev)
>          return -ENOMEM;
>      }
>
> +    this->buffer = devm_kzalloc(&pdev->dev, strlen(fingerprint), GFP_KERNEL);
> +    if (!this->buffer)
> +        return -ENOMEM;
> +
>      platform_set_drvdata(pdev, this);
>      this->pdev  = pdev;
>      this->dev   = &pdev->dev;
...

As Huang noted, this approach is still very fragile. I think it's best
if we rework GPMI's init sequence so that we don't have to do these
kind of fragile dodges any more; just do all the BBT scanning after
nand_scan_tail(). Let's wait for Huang's version of my suggestion.

Brian