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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>
> ---
>
>  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@xxxxxxxxxxxxxx/



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux