Re: [PATCH v3] wmi/core: fix use-after-free in wmi_add_device()

[Armin Wolf]

Am 22.06.26 um 21:18 schrieb yahia:

> From: yahia ahmed <yahia.a.abdrabou@xxxxxxxxx>
>
> Fix a use-after-free error in wmi_add_device()
> where if a device_add() fails, it will forward
> the error code to the caller function parse_wdg(),
> which then frees wblock, but doesn't remove it From
> pdev's list, thus if pdev were to call it, it will
> call freed memory and result in a use-after-free
>
> Signed-off-by: yahia ahmed <yahia.a.abdrabou@xxxxxxxxx>
> ---
> v3:
>    - Fix corruption in v2
> v2:
>    -  Fix use-after-free in wmi_add_device() by using device_link_del()
>   drivers/platform/wmi/core.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
> index 529825dcfbfe..1afc4d12bdfb 100644
> --- a/drivers/platform/wmi/core.c
> +++ b/drivers/platform/wmi/core.c
> @@ -1268,6 +1268,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>   static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
>   {
>   	struct device_link *link;
> +	int ret;
>   
>   	/*
>   	 * Many aggregate WMI drivers do not use -EPROBE_DEFER when they
> @@ -1282,7 +1283,11 @@ static int wmi_add_device(struct platform_device *pdev, struct wmi_device *wdev)
>   	if (!link)
>   		return -EINVAL;
>   
> -	return device_add(&wdev->dev);
> +	ret = device_add(&wdev->dev);
> +
> +	if (ret)
> +		device_link_del(link);

The documentation of device_link_add() states that:

	If that flag is not set, however, the caller of this function is handing the
	management of the link over to the driver core entirely and its return value
	can only be used to check whether or not the link is present.  In that case,
	the DL_FLAG_AUTOREMOVE_CONSUMER and DL_FLAG_AUTOREMOVE_SUPPLIER device link
	flags can be used to indicate to the driver core when the link can be safely
	deleted.  Namely, setting one of them in @flags indicates to the driver core
	that the link is not going to be used (by the given caller of this function)
	after unbinding the consumer or supplier driver, respectively, from its
	device, so the link can be deleted at that point.  If none of them is set,
	the link will be maintained until one of the devices pointed to by it (either
	the consumer or the supplier) is unregistered.

According to my understanding the device link will be freed automatically should device_add() fail.

Thanks,
Armin Wolf

> +	return ret;
>   }
>   
>   /*