linux-kernel - Re: [PATCH] usb: dwc3: dwc3-qcom: Avoid use-after-free when USB defers or unbinds

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:   Mon, 6 Dec 2021 16:37:28 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Douglas Anderson <dianders@...omium.org>,
        Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Sandeep Maheswaram <quic_c_sanm@...cinc.com>,
        Wesley Cheng <quic_wcheng@...cinc.com>, robdclark@...omium.org,
        linux-arm-msm@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Wesley Cheng <wcheng@...eaurora.org>,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH] usb: dwc3: dwc3-qcom: Avoid use-after-free when USB
 defers or unbinds

Quoting Douglas Anderson (2021-12-06 15:28:47)
> On sc7180-trogdor class devices with 'fw_devlink=permissive' and KASAN
> enabled, you'll see a Use-After-Free reported at bootup.
>
> The root of the problem is that dwc3_qcom_of_register_core() is adding
> a devm-allocated "tx-fifo-resize" property to its device tree node
> using of_add_property().
>
> The issue is that of_add_property() makes a _permanent_ addition to
> the device tree that lasts until reboot. That means allocating memory
> for the property using "devm" managed memory is a terrible idea since
> that memory will be freed upon probe deferral or device
> unbinding. Let's change to just allocate memory once and never free
> it. This sorta looks like a leak but isn't truly one, since only one
> property will be allocated per device tree node per boot.
>
> NOTE: one would think that perhaps it would be better to use
> of_remove_property() and then be able to free the property on device
> remove. That sounds good until you read the comments for
> of_remove_property(), which says that properties are never really
> removed and they're just moved to the side.

Is it a problem to remove and then add again? It moves it to the side
which means we may run out of memory?

>
> Fixes: cefdd52fa045 ("usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
>  drivers/usb/dwc3/dwc3-qcom.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 9abbd01028c5..34b054033116 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -658,18 +658,28 @@ static int dwc3_qcom_of_register_core(struct platform_device *pdev)
>                 return -ENODEV;
>         }
>
> -       prop = devm_kzalloc(dev, sizeof(*prop), GFP_KERNEL);
> -       if (!prop) {
> -               ret = -ENOMEM;
> -               dev_err(dev, "unable to allocate memory for property\n");
> -               goto node_put;
> -       }
> +       /*
> +        * Permanently add the "tx-fifo-resize" to the device tree. Even if
> +        * our device is unregistered this property will still be part
> +        * of the device tree until reboot. Because this is a "permanent"
> +        * change, we allocate memory _without_ devm. For some context, see
> +        * the fact that of_remove_property() doesn't actually remove things.
> +        */
> +       if (!of_find_property(dwc3_np, "tx-fifo-resize", NULL)) {
> +               prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> +               if (!prop) {
> +                       ret = -ENOMEM;
> +                       dev_err(dev, "unable to allocate memory for property\n");

Allocations already print a big error when they fail to allocate. Please
drop this error message.

> +                       goto node_put;
> +               }
>
> -       prop->name = "tx-fifo-resize";
> -       ret = of_add_property(dwc3_np, prop);

I don't understand why we can't tell dwc3 that we want to use
tx-fifo-resize without adding a DT property. DT isn't the only way we
could probe this qcom dwc3 device, there's also ACPI. And in dwc3 core
where we check for this property couldn't we add a compatible check for
qcom,dwc3 and then force the property? I see that a lot of this was
already discussed when these patches got applied by gregkh directly[1].

Can we revert out this bad code instead?

> -       if (ret) {
> -               dev_err(dev, "unable to add property\n");
> -               goto node_put;
> +               prop->name = "tx-fifo-resize";
> +               ret = of_add_property(dwc3_np, prop);

[1] https://lore.kernel.org/all/b5917fc0-c916-0a51-dc4c-315d7f02cafa@codeaurora.org/