linux-kernel - Re: [PATCH] usb: ehci-orion: Handle errors of clk_prepare

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:   Fri, 27 Aug 2021 14:51:56 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Evgeny Novikov <novikov@...ras.ru>
Cc:     Alan Stern <stern@...land.harvard.edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Andrew Lunn <andrew@...n.ch>,
        Mike Turquette <mturquette@...aro.org>,
        Kirill Shilimanov <kirill.shilimanov@...wei.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        ldv-project@...uxtesting.org
Subject: Re: [PATCH] usb: ehci-orion: Handle errors of clk_prepare_enable()
 in probe

On Thu, Aug 26, 2021 at 07:26:22PM +0300, Evgeny Novikov wrote:
> I added Dan to the discussion since he is a developer of one of such
> tools.

Thanks for that...  :P

I never warn about "forgot to check the return" bugs except in the case
of error pointers or allocation failures.  There are too many false
positives.  If people want to do that they should add a __must_check
attribute to the function.

You linked to another thread: https://lkml.org/lkml/2021/8/17/239

That patch isn't correct.  Miquel was on the right track but not 100%.
The nand_scan() calls mxic_nfc_clk_enable() so we should disable it
until it has been successfully enabled.  The current code can trigger a
WARN() in __clk_disable().  In other words it should have been:

diff --git a/drivers/mtd/nand/raw/mxic_nand.c b/drivers/mtd/nand/raw/mxic_nand.c
index da1070993994..87aef98f5b9d 100644
--- a/drivers/mtd/nand/raw/mxic_nand.c
+++ b/drivers/mtd/nand/raw/mxic_nand.c
@@ -535,11 +535,11 @@ static int mxic_nfc_probe(struct platform_device *pdev)
 	err = devm_request_irq(&pdev->dev, irq, mxic_nfc_isr,
 			       0, "mxic-nfc", nfc);
 	if (err)
-		goto fail;
+		return err;

 	err = nand_scan(nand_chip, 1);
 	if (err)
-		goto fail;
+		return err;

 	err = mtd_device_register(mtd, NULL, 0);
 	if (err)

nand_scan() error handling is still leaky, but it's hard to fix without
re-working the API.

Anyway, thanks for the fixes.  I've been inspired by the Linux Driver
Verification project work.

regards,
dan carpenter