mtd: gpmi: Fix NULL pointer dereference

From: Fabio Estevam <[email protected]>

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 <[email protected]> # 3.12

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +1 -1

index a9830ff..e090280 100644
@@ -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;
 

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +4 -0

+	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

(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.

Quoted 6 lines · 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().

于 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().

Quoted 14 lines · Huang Shijie

于 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.

Quoted 3 lines

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:

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +2 -3

index a9830ff..647da1b 100644
@@ -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;

          }

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +0 -1

@@ -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;

Quoted 15 lines · Huang Shijie

于 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.

Quoted 3 lines

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:

drivers/mtd/nand/gpmi-nand/gpmi-nand.c +0 -0

index a9830ff..647da1b 100644