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@xxxxxxxxxx>
Regards,
Bjorn
> 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");
> + 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
>
