Message-ID: <CAD=FV=W2M0=7LG4g-si1ce_rnhP9DWW6mm1Vb5aUkFcC3TtYng@mail.gmail.com>
Date:   Mon, 6 Dec 2021 16:48:47 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        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

Hi,

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

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/

Powered by blists - more mailing lists