linux-kernel - Re: [PATCH] spi: bcm-qspi: return error if neither hif

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]

Message-ID: <CAKekbeuMjUPpzfgKrxgZzFpiQ4FbeYtBtbrzkWKeBy4u2Symhg@mail.gmail.com>
Date:   Thu, 29 Jun 2023 11:06:44 -0400
From:   Kamal Dasu <kamal.dasu@...adcom.com>
To:     Jonas Gorski <jonas.gorski@...il.com>
Cc:     Kamal Dasu <kdasu.kdev@...il.com>,
        Broadcom internal kernel review list 
        <bcm-kernel-feedback-list@...adcom.com>,
        Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: bcm-qspi: return error if neither hif_mspi nor mspi
 is available

On Thu, Jun 29, 2023 at 9:43 AM Jonas Gorski <jonas.gorski@...il.com> wrote:
>
> If neither a "hif_mspi" nor "mspi" resource is present, the driver will
> just early exit in probe but still return success. Apart from not doing
> anything meaningful, this would then also lead to a null pointer access
> on removal, as platform_get_drvdata() would return NULL, which it would
> then try to dereferce when trying to unregister the spi master.
>
> Fix this by unconditionally calling devm_ioremap_resource(), as it can
> handle a NULL res and will then return a viable ERR_PTR() if we get one.
>
> The "return 0;" was previously a "goto qspi_resource_err;" where then
> ret was returned, but since ret was still initialized to 0 at this place
> this was a valid conversion in 63c5395bb7a9 ("spi: bcm-qspi: Fix
> use-after-free on unbind"). The issue was not introduced by this commit,
> only made more obvious.
>
> Fixes: fa236a7ef240 ("spi: bcm-qspi: Add Broadcom MSPI driver")
> Signed-off-by: Jonas Gorski <jonas.gorski@...il.com>
> ---
> Found by looking a the driver while comparing it to its bindings.
>
> Only build tested, not runtested.
>
>  drivers/spi/spi-bcm-qspi.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/spi-bcm-qspi.c b/drivers/spi/spi-bcm-qspi.c
> index 6b46a3b67c41..d91dfbe47aa5 100644
> --- a/drivers/spi/spi-bcm-qspi.c
> +++ b/drivers/spi/spi-bcm-qspi.c
> @@ -1543,13 +1543,9 @@ int bcm_qspi_probe(struct platform_device *pdev,
>                 res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>                                                    "mspi");
>
> -       if (res) {
> -               qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
> -               if (IS_ERR(qspi->base[MSPI]))
> -                       return PTR_ERR(qspi->base[MSPI]);
> -       } else {
> -               return 0;
> -       }

I would rather just do this in the else case

} else {
 -              return 0;
 +             return -ENODEV;
}

 The change below does not check the return of
platform_get_resource_byname() in my opinion rather relies on
devm_ioremap_resource() doing the right thing.

> +       qspi->base[MSPI]  = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(qspi->base[MSPI]))
> +               return PTR_ERR(qspi->base[MSPI]);
>
>         res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "bspi");
>         if (res) {
> --
> 2.34.1
>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4203 bytes)