Hi Fabio, Huang,
On Mon, Nov 11, 2013 at 03:08:47PM -0200, Fabio Estevam wrote:
> From: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx> # 3.12
> Signed-off-by: Fabio Estevam <fabio.estevam@xxxxxxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html