linux-kernel - Re: [PATCH] mtd: rawnand: check return code of nand_reset() and nand_readid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Jan 2019 14:14:03 +0100
From:   Boris Brezillon <bbrezillon@...nel.org>
To:     Masahiro Yamada <yamada.masahiro@...ionext.com>
Cc:     linux-mtd@...ts.infradead.org,
        Boris Brezillon <boris.brezillon@...tlin.com>,
        Miquel Raynal <miquel.raynal@...tlin.com>,
        Marek Vasut <marek.vasut@...il.com>,
        Richard Weinberger <richard@....at>,
        linux-kernel@...r.kernel.org,
        Brian Norris <computersforpeace@...il.com>,
        David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH] mtd: rawnand: check return code of nand_reset() and
 nand_readid_op()

On Mon, 21 Jan 2019 22:05:34 +0900
Masahiro Yamada <yamada.masahiro@...ionext.com> wrote:

> nand_scan_ident() iterates over maxchips to find as many homogeneous
> chips as possible.
> 
> Currently, this loop bails out only when manufacturer or device ID
> unmatches. The reason of unmatch is most likely no chip is connected
> to that chip select. In this case, nand_reset() has already failed,
> and the following nand_readid_op() is pointless.

While I agree with the following diff, I'd also like to point out that
nand_scan() callers should know how many controller CS lines are
connected to the chip (board file or DT description). The check we do in
nand_scan_ident() should only be here to clamp this value if the board
desc is wrong (maybe we should even fail in that case instead of
silently fixing things).

> 
> Before ->exec_op hook was introduced, drivers had no way to tell
> the failure of NAND_CMD_RESET to the framework because the legacy
> ->cmdfunc() has void return type. Now drivers implementing ->exec_op  
> hook can return the error code. You can save nand_readid_op() by
> checking the return value of nand_reset(). The return value of
> nand_readid_op() should be checked as well. If it fails, probably
> id[0] and id[1] are undefined values.
> 
> Just for consistency, it should be sensible to check the return
> code in nand_do_write_oob() as well.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@...ionext.com>

Reviewed-by: Boris Brezillon <bbrezillon@...nel.org>

> ---
> 
>  drivers/mtd/nand/raw/nand_base.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 7ea3f10..3407523 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -457,7 +457,7 @@ static int nand_do_write_oob(struct nand_chip *chip, loff_t to,
>  			     struct mtd_oob_ops *ops)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> -	int chipnr, page, status, len;
> +	int chipnr, page, status, len, ret;
>  
>  	pr_debug("%s: to = 0x%08x, len = %i\n",
>  			 __func__, (unsigned int)to, (int)ops->ooblen);
> @@ -479,7 +479,9 @@ static int nand_do_write_oob(struct nand_chip *chip, loff_t to,
>  	 * if we don't do this. I have no clue why, but I seem to have 'fixed'
>  	 * it in the doc2000 driver in August 1999.  dwmw2.
>  	 */
> -	nand_reset(chip, chipnr);
> +	ret = nand_reset(chip, chipnr);
> +	if (ret)
> +		return ret;
>  
>  	nand_select_target(chip, chipnr);
>  
> @@ -5037,11 +5039,15 @@ static int nand_scan_ident(struct nand_chip *chip, unsigned int maxchips,
>  		u8 id[2];
>  
>  		/* See comment in nand_get_flash_type for reset */
> -		nand_reset(chip, i);
> +		ret = nand_reset(chip, i);
> +		if (ret)
> +			break;
>  
>  		nand_select_target(chip, i);
>  		/* Send the command for reading device ID */
> -		nand_readid_op(chip, 0, id, sizeof(id));
> +		ret = nand_readid_op(chip, 0, id, sizeof(id));
> +		if (ret)
> +			break;
>  		/* Read manufacturer and device IDs */
>  		if (nand_maf_id != id[0] || nand_dev_id != id[1]) {
>  			nand_deselect_target(chip);