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/
