On Thu, Jun 29, 2023 at 11:38 AM Jonas Gorski <jonas.gorski@xxxxxxxxx> wrote:
>
> On Thu, 29 Jun 2023 at 17:07, Kamal Dasu <kamal.dasu@xxxxxxxxxxxx> wrote:
> >
> > On Thu, Jun 29, 2023 at 9:43 AM Jonas Gorski <jonas.gorski@xxxxxxxxx> 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.
s/dereferce/ dereference
> > >
> > > 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@xxxxxxxxx>
> > > ---
Reviewed-by: Kamal Dasu <kamal.dasu@xxxxxxxxxxxx>
> > > 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.
>
> This is how devm_ioremap_resource() is intended to be used, see e.g.
> the example in its kernel documentation:
>
> https://elixir.bootlin.com/linux/latest/source/lib/devres.c#L167
>
> So I don't see what's wrong with relying on functions doing the right thing.
>
> Also AFAIU the appropriate return code in this case would be rather
> -EINVAL, not -ENODEV.
>
> >
> > > + 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
> > >
>
> Regards,
> Jonas
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
