mtd: gpmi: Fix NULL pointer dereference

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:

Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

于 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

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