Re: [PATCH v2] bus: hisi_lpc: Don't use devm_kzalloc() to allocate logical PIO range

[Bjorn Helgaas]

On Fri, Jun 14, 2019 at 8:47 AM John Garry <john.garry@xxxxxxxxxx> wrote:
> On 14/06/2019 14:23, Bjorn Helgaas wrote:
> > On Fri, Jun 14, 2019 at 8:20 AM Bjorn Helgaas <bhelgaas@xxxxxxxxxx> wrote:
> >> On Fri, Jun 14, 2019 at 5:26 AM John Garry <john.garry@xxxxxxxxxx> wrote:
> >>>
> >>> If, after registering a logical PIO range, the driver probe later fails,
> >>> the logical PIO range memory will be released automatically.
> >>>
> >>> This causes an issue, in that the logical PIO range is not unregistered
> >>> (that is not supported) and the released range memory may be later
> >>> referenced
> >>>
> >>> Allocate the logical PIO range with kzalloc() to avoid this potential
> >>> issue.
> >>>
> >>> Fixes: adf38bb0b5956 ("HISI LPC: Support the LPC host on Hip06/Hip07 with DT bindings")
> >>> Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
> >>> ---
> >>>
> >>> Change to v1:
> >>> - add comment, as advised by Joe Perches
> >>>
> >>> diff --git a/drivers/bus/hisi_lpc.c b/drivers/bus/hisi_lpc.c
> >>> index 19d7b6ff2f17..5f0130a693fe 100644
> >>> --- a/drivers/bus/hisi_lpc.c
> >>> +++ b/drivers/bus/hisi_lpc.c
> >>> @@ -599,7 +599,8 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> >>>         if (IS_ERR(lpcdev->membase))
> >>>                 return PTR_ERR(lpcdev->membase);
> >>>
> >>> -       range = devm_kzalloc(dev, sizeof(*range), GFP_KERNEL);
> >>> +       /* Logical PIO may reference 'range' memory even if the probe fails */
> >>> +       range = kzalloc(sizeof(*range), GFP_KERNEL);
> >>
> >> This doesn't feel like the correct way to fix this.  If the probe
> >> fails, everything done by the probe should be undone, including the
> >> allocation and registration of "range".  I don't know what the best
> >> mechanism is, but I'm skeptical about this one.
>
> I understand your concern.
>
> For the logical PIO framework, it was written to match what was done
> originally for PCI IO port management in pci_register_io_range(), cf
> https://elixir.bootlin.com/linux/v4.4.180/source/drivers/of/address.c#L691
>
> That is, no method to unregister ranges. As such, it leaks IO port
> ranges. I can come up with a few guesses why the original PCI IO port
> management author did not add an unregistration method.

I think that was written before the era of support for hot-pluggable
host bridges and loadable drivers for them.

> Anyway, we can work on adding support to unregister regions, at least at
> probe time. It may become more tricky to do this once the host children
> have probed and are accessing the IO port regions.

I think we *do* need support for unregistering regions because we do
claim to support hot-pluggable host bridges, and the I/O port regions
below them should go away when the host bridge does.

Could you just move the logic_pio_register_range() call farther down
in hisi_lpc_probe()?  IIUC, once logic_pio_register_range() returns,
an inb() with the right port number will try to access that port, so
we should be prepared for that, i.e., maybe this in the wrong order to
begin with?

> But, for now, I would like to get this simple fix added to mainline and
> backported.

OK, I don't really like code that looks "obviously wrong" but has
extenuating circumstances that need explanation because it sows
confusion and can get copied elsewhere.  But I'm just kibbitzing here
since I don't maintain this, so it's up to you.

> FYI, there should be no possible further memory leak for continuously
> probing the driver.

That's good.

You might consider a subject line that includes key words like "avoid
use-after-free" or some similar higher-level description that tells
the reader *why* this change is important.

Bjorn

> >>>         if (!range)
> >>>                 return -ENOMEM;
> >>>
> >>> @@ -610,6 +611,7 @@ static int hisi_lpc_probe(struct platform_device *pdev)
> >>>         ret = logic_pio_register_range(range);
> >>>         if (ret) {
> >>>                 dev_err(dev, "register IO range failed (%d)!\n", ret);
> >>> +               kfree(range);
> >>>                 return ret;
> >>>         }
> >>>         lpcdev->io_host = range;
> >>> --
> >>> 2.17.1
> >>>
> >
> > .
> >
>
>