| Message ID | 20211206152844.1.I411110cc99c1dd66b01aa9aa25651acf8ff55da1@changeid (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | usb: dwc3: dwc3-qcom: Avoid use-after-free when USB defers or unbinds | expand |
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 >
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>
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/
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/
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/
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
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);
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(-)