On Fri, Jun 14, 2019 at 8:47 AM John Garry <john.ga...@huawei.com> wrote:
> On 14/06/2019 14:23, Bjorn Helgaas wrote:
> > On Fri, Jun 14, 2019 at 8:20 AM Bjorn Helgaas <bhelg...@google.com> wrote:
> >> On Fri, Jun 14, 2019 at 5:26 AM John Garry <john.ga...@huawei.com> 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.ga...@huawei.com>
> >>> ---
> >>>
> >>> 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
> >>>
> >
> > .
> >
>
>
