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