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

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, 14 Jun 2019 08:20:10 -0500
From:   Bjorn Helgaas <bhelgaas@...gle.com>
To:     John Garry <john.garry@...wei.com>
Cc:     xuwei5@...wei.com, linuxarm@...wei.com, arm@...nel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Joe Perches <joe@...ches.com>,
        Zhichang Yuan <yuanzhichang@...ilicon.com>
Subject: Re: [PATCH v2] bus: hisi_lpc: Don't use devm_kzalloc() to allocate
 logical PIO range

[+cc Zhichang, logic_pio author]

On Fri, Jun 14, 2019 at 5:26 AM John Garry <john.garry@...wei.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.garry@...wei.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.

>         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
>