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

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

[Douglas Anderson]

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.

Fixes: cefdd52fa045 ("usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default")
Signed-off-by: Douglas Anderson <dianders@chromium.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");
+			goto node_put;
+		}
 
-	prop->name = "tx-fifo-resize";
-	ret = of_add_property(dwc3_np, prop);
-	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);
+		if (ret) {
+			dev_err(dev, "unable to add property\n");
+			kfree(prop);
+			goto node_put;
+		}
 	}
 
 	ret = of_platform_populate(np, NULL, NULL, dev);
-- 
2.34.1.400.ga245620fadb-goog

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

[Bjorn Andersson]

On Mon 06 Dec 15:28 PST 2021, Douglas Anderson wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Fixes: cefdd52fa045 ("usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default")
> Signed-off-by: Douglas Anderson <dianders@chromium.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");
> +			goto node_put;
> +		}
>  
> -	prop->name = "tx-fifo-resize";
> -	ret = of_add_property(dwc3_np, prop);
> -	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);
> +		if (ret) {
> +			dev_err(dev, "unable to add property\n");
> +			kfree(prop);
> +			goto node_put;
> +		}
>  	}
>  
>  	ret = of_platform_populate(np, NULL, NULL, dev);
> -- 
> 2.34.1.400.ga245620fadb-goog
> 

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

[Matthias Kaehlcke]

On Mon, Dec 06, 2021 at 03:28:47PM -0800, Douglas Anderson wrote:
> 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.
> 
> Fixes: cefdd52fa045 ("usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>

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

[Stephen Boyd]

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@chromium.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/

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

[Doug Anderson]

Hi,

On Mon, Dec 6, 2021 at 4:37 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> 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?

I suspect it would mostly work to do it, but to me it seems worse than
the solution I have here. Specifically, I presume of_remove_property()
is written to "move things to the side" for a reason. I guess in
general callers are expecting that they can just hold onto properties
forever, so if someone happened to decide to hold onto our property
then things could go boom when we free it.

Oh, actually, looking at the implementation I think there's another
problem. I believe when of_remove_property() moves the property to the
side it still expects that it can use the linked list node pointers in
the property to chain it onto the "deadprops" list.


> > Fixes: cefdd52fa045 ("usb: dwc3: dwc3-qcom: Enable tx-fifo-resize property by default")
> > Signed-off-by: Douglas Anderson <dianders@chromium.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.

Makes sense to drop it. I can post a v2 with this or a follow-up patch
depending on what people want. For now I'll wait until we get
agreement on what we want to do with this patch.


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

I'm OK w/ just reverting the bad code, if that's what people want.


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

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

[Bjorn Andersson]

On Mon 06 Dec 16:37 PST 2021, Stephen Boyd wrote:
> Quoting Douglas Anderson (2021-12-06 15:28:47)
[..]
> > +                       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].
> 

When the tx-fifo-resize property was introduced I made an effort to
convince the people involved about the prospect of passing this
information in the code, rather than using DT as some sort of parameter
store to pass information between the devices.

And I still would like us to come up with some sort of code-level
mechanism for passing some state between dwc3-qcom and the dwc3-core,
because I really want to register some callback with the core so that we
don't need to duplicate extcon and usb_role_switch in both the core and
platform glue.

See this discussion:
https://lore.kernel.org/linux-arm-msm/YSZCmDEedJaJyI0u@ripper/

> Can we revert out this bad code instead?
> 

You definitely have my vote for that!

Regards,
Bjorn

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

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

[Doug Anderson]

Hi,

On Mon, Dec 6, 2021 at 6:32 PM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Mon 06 Dec 16:37 PST 2021, Stephen Boyd wrote:
> > Quoting Douglas Anderson (2021-12-06 15:28:47)
> [..]
> > > +                       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].
> >
>
> When the tx-fifo-resize property was introduced I made an effort to
> convince the people involved about the prospect of passing this
> information in the code, rather than using DT as some sort of parameter
> store to pass information between the devices.
>
> And I still would like us to come up with some sort of code-level
> mechanism for passing some state between dwc3-qcom and the dwc3-core,
> because I really want to register some callback with the core so that we
> don't need to duplicate extcon and usb_role_switch in both the core and
> platform glue.
>
> See this discussion:
> https://lore.kernel.org/linux-arm-msm/YSZCmDEedJaJyI0u@ripper/
>
> > Can we revert out this bad code instead?
> >
>
> You definitely have my vote for that!

Sure. I've posted a revert up, so either the revert or ${SUBJECT}
patch will fix the problem. For the revert, see:

https://lore.kernel.org/r/20211207094327.1.Ie3cde3443039342e2963262a4c3ac36dc2c08b30@changeid

-Doug