linux-kernel - Re: [PATCH v2] mtd: bcm47xxnflash: Add error handling for bcm47xxnflash_ops_bcm4706_ctl

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 for Android: free password hash cracker in your pocket

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

Message-ID: <875xjgihwr.fsf@bootlin.com>
Date: Mon, 07 Apr 2025 08:51:00 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Wentao Liang <vulab@...as.ac.cn>
Cc: richard@....at,  vigneshr@...com,  linux-mtd@...ts.infradead.org,
  linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] mtd: bcm47xxnflash: Add error handling for
 bcm47xxnflash_ops_bcm4706_ctl_cmd()

Hello Liang,

On 05/04/2025 at 22:33:47 +08, Wentao Liang <vulab@...as.ac.cn> wrote:

> The bcm47xxnflash_ops_bcm4706_cmd_ctrl() calls
> bcm47xxnflash_ops_bcm4706_ctl_cmd() without printing error message. A
> proper implementation can be found in
> bcm47xxnflash_ops_bcm4706_write().

This is a problem with cmd_ctrl, but this interface has been deprecated
~5y ago and people encouraged to transition to ->exec_op(). Did you
personally experience issues with that or is this just code analysis? If
you are actually using this driver, I'd highly suggest to update the
driver.

> Add error log to the bcm47xxnflash_ops_bcm4706_ctl_cmd() to prevent
> silent failure.
>
> Signed-off-by: Wentao Liang <vulab@...as.ac.cn>
> ---
>  drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> index 6487dfc64258..c89129588bb6 100644
> --- a/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> +++ b/drivers/mtd/nand/raw/bcm47xxnflash/ops_bcm4706.c
> @@ -182,7 +182,8 @@ static void bcm47xxnflash_ops_bcm4706_cmd_ctrl(struct nand_chip *nand_chip,
>  	if (cmd != NAND_CMD_RESET)
>  		code |= NCTL_CSA;
>  
> -	bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code);
> +	if (bcm47xxnflash_ops_bcm4706_ctl_cmd(b47n->cc, code))
> +		pr_err("%s ctl_cmd didn't work!\n", __func__);

dev_err() is probably better
__func__ is not needed
If we want to make it a bit usable, I'd say that the return code shall
be there as well.

>  }
>  
>  /* Default nand_select_chip calls cmd_ctrl, which is not used in BCM4706 */

Thanks,
Miquèl