[PATCH] wmi/core Fix use-after-free in parse_wdg()

[PATCH] wmi/core Fix use-after-free in parse_wdg()

[yahia]

From: yahia ahmed <yahia.a.abdrabou@gmail.com>

Fix use-after-free in parse_wdg() function by
setting wblock to null to prevent referencing 
freed memory, if wblock is freed in the if 
(retval) path and address a minor typo.

Signed-off-by: yahia ahmed <yahia.a.abdrabou@gmail.com>
---
 drivers/platform/wmi/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
index 5a2ffcbab6af..9b5bd9ef5ba2 100644
--- a/drivers/platform/wmi/core.c
+++ b/drivers/platform/wmi/core.c
@@ -1328,6 +1328,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
 		retval = wmi_create_device(wmi_bus_dev, wblock, device);
 		if (retval) {
 			kfree(wblock);
+			wblock = NULL; /* Set wblock to NULL to prevent a use after free */
 			continue;
 		}
 
@@ -1418,7 +1419,7 @@ static int wmi_notify_device(struct device *dev, void *data)
 		return 0;
 
 	/* The ACPI WMI specification says that _WED should be
-	 * evaluated every time an notification is received, even
+	 * evaluated every time a notification is received, even
 	 * if no consumers are present.
 	 *
 	 * Some firmware implementations actually depend on this
-- 
2.54.0

Re: [PATCH] wmi/core Fix use-after-free in parse_wdg()

[Armin Wolf]

Am 22.06.26 um 02:02 schrieb yahia:

> From: yahia ahmed <yahia.a.abdrabou@gmail.com>
>
> Fix use-after-free in parse_wdg() function by
> setting wblock to null to prevent referencing
> freed memory, if wblock is freed in the if
> (retval) path and address a minor typo.

Hi,

where exactly does this UAF happen? Keep in mind that kmalloc_obj()
only "dereferences" *wblock to use sizeof() on the type pointed to by
the pointer.

Thanks,
Armin Wolf

> Signed-off-by: yahia ahmed <yahia.a.abdrabou@gmail.com>
> ---
>   drivers/platform/wmi/core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
> index 5a2ffcbab6af..9b5bd9ef5ba2 100644
> --- a/drivers/platform/wmi/core.c
> +++ b/drivers/platform/wmi/core.c
> @@ -1328,6 +1328,7 @@ static int parse_wdg(struct device *wmi_bus_dev, struct platform_device *pdev)
>   		retval = wmi_create_device(wmi_bus_dev, wblock, device);
>   		if (retval) {
>   			kfree(wblock);
> +			wblock = NULL; /* Set wblock to NULL to prevent a use after free */
>   			continue;
>   		}
>   
> @@ -1418,7 +1419,7 @@ static int wmi_notify_device(struct device *dev, void *data)
>   		return 0;
>   
>   	/* The ACPI WMI specification says that _WED should be
> -	 * evaluated every time an notification is received, even
> +	 * evaluated every time a notification is received, even
>   	 * if no consumers are present.
>   	 *
>   	 * Some firmware implementations actually depend on this

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

[yahia]

From: yahia ahmed <yahia.a.abdrabou@gmail.com>

Hi Armin,

You are correct about kzalloc_obj overriding wblock and
I apologize, However there is a different use-after-free
specifically in wmi_add_device() function, where if device_add()
fails, the function forwards the return code, then the caller
function parse_wdg() frees wblock from memory, but doesnt remove
wblock from pdev's list, thus if pdev calls wblock for any reason
like suspendension or similar activities, it will call freed memory,
hence the use-after-free.

I propose to fix this by adding device_link_del() function
if device_add() function fails, remove wblock from pdev's
list, thus avoiding a possible use-after-free.

Signed-off-by: yahia ahmed <yahia.a.abdrabou@gmail.com>
---
 drivers/platform/wmi/core.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
index 5a2ffcbab6af..8c9b7ac2e5d8 100644
--- a/drivers/platform/wmi/core.c
+++ b/drivers/platform/wmi/core.c
@@ -1261,6 +1261,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
@@ -1275,7 +1276,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);
+	return ret;
 }
 
@@ -1418,7 +1424,7 @@ static int wmi_notify_device(struct device *dev, void *data)
 		return 0;
 
 	/* The ACPI WMI specification says that _WED should be
-	 * evaluated every time an notification is received, even
+	 * evaluated every time a notification is received, even
 	 * if no consumers are present.
 	 *
 	 * Some firmware implementations actually depend on this
-- 
2.54.0

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

[yahia]

From: yahia ahmed <yahia.a.abdrabou@gmail.com>

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@gmail.com>
---
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);
+	return ret;
 }
 
 /*
-- 
2.54.0

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@gmail.com>
>
> 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@gmail.com>
> ---
> 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;
>   }
>   
>   /*

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

[yahia]

Hi Armin,

As per the documentation, The automatic cleanup is only
triggered if the supplier probes or unbinds

      Similarly, when the device link is added from supplier's ``->probe`` callback,
      ``DL_FLAG_AUTOREMOVE_SUPPLIER`` causes the device link to be automatically
      purged when the supplier fails to probe or later unbinds.

To my understanding, the cleanup won't activate
here because we don't unbind pdev nor does the
probe fail.

Best regards,
yahia

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

[Armin Wolf]

Am 22.06.26 um 22:25 schrieb yahia:

> Hi Armin,
>
> As per the documentation, The automatic cleanup is only
> triggered if the supplier probes or unbinds
>
>        Similarly, when the device link is added from supplier's ``->probe`` callback,
>        ``DL_FLAG_AUTOREMOVE_SUPPLIER`` causes the device link to be automatically
>        purged when the supplier fails to probe or later unbinds.
>
> To my understanding, the cleanup won't activate
> here because we don't unbind pdev nor does the
> probe fail.
>
> Best regards,
> yahia

True, but the associated wdev will be kept alive by the device link reference.
The cleanup will then happen when the platform driver unbinds.

So IMHO no UAF is taking place here, but we could optimize this by dropping
the flag to let the driver core to delete the device link earlier.

Thanks,
Armin Wolf

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

[yahia]

Hi Armin,

Thank you for your clarification, I am
completely on board with the idea of changing
the flags though I dont get whether you mean
change DL_FLAG_AUTOREMOVE_SUPPLIER in favor
of DL_FLAG_AUTOREMOVE_CONSUMER or do you have
a different idea in mind?

Best regards,
yahia

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

[Armin Wolf]

Am 22.06.26 um 22:57 schrieb yahia:

> Hi Armin,
>
> Thank you for your clarification, I am
> completely on board with the idea of changing
> the flags though I dont get whether you mean
> change DL_FLAG_AUTOREMOVE_SUPPLIER in favor
> of DL_FLAG_AUTOREMOVE_CONSUMER or do you have
> a different idea in mind?
>
> Best regards,
> yahia

DL_FLAG_AUTOREMOVE_CONSUMER should not be used because the device link needs to
remain even after a child WMI device unbinds his driver.

I thought about simply dropping DL_FLAG_AUTOREMOVE_SUPPLIER, but i would be very
happy if you could test that this actually works.

Thanks,
Armin Wolf

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

[yahia]

Hi Armin,

I compiled the kernel while dropping
HL_FLAG_AUTOREMOVE_SUPPLIER and the kernel
functioned normally, all my hotkeys worked
and i couldn't find any memory leak or issue
with KASAN, so in my testing it seems safe to
drop HL_FLAG_AUTOREMOVE_SUPPLIER.

Best regards,
yahia

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

[Armin Wolf]

Am 23.06.26 um 02:12 schrieb yahia:

> Hi Armin,
>
> I compiled the kernel while dropping
> HL_FLAG_AUTOREMOVE_SUPPLIER and the kernel
> functioned normally, all my hotkeys worked
> and i couldn't find any memory leak or issue
> with KASAN, so in my testing it seems safe to
> drop HL_FLAG_AUTOREMOVE_SUPPLIER.
>
> Best regards,
> yahia
>
Thanks for testing this. I did a similar test where i also removed the DL_FLAG_AUTOREMOVE_SUPPLIER
flag. However i also modified wmi_add_device() to always fail with -EINVAL.

This resulted in the following error when the WMI driver was unbound and later rebound to the same
PNP0C14 device:

kobject: kobject_add_internal failed for platform:PNP0C14:00--wmi:C3021213-D0BC-41A2-BA17-816CD5ED7744-0 with -EEXIST, don't try to register things with the same name in the same directory.

It seems that without DL_FLAG_AUTOREMOVE_SUPPLIER, all device links will remain even after the WMI core driver
has been unbound from a given device. This causes a name clash when said driver is bound again to the same
device.

Because of this i do not think that we should remove this flag. It seems that the driver core does not remove the link
automatically if the consumer device was not successfully added using device_add().

Thanks,
Armin Wolf

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

[yahia]

Hi Armin,

Thanks for clarifying the design
on the design interaction

Best regards,
yahia

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

[Ilpo Järvinen]

On Mon, 22 Jun 2026, yahia wrote:

> From: yahia ahmed <yahia.a.abdrabou@gmail.com>

So do I get it right that the conclusion of the discussion was that this 
still required?

If that's the case, here's things that need to be fixed:

> 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

Please add the final stop (.)

The lines are too short, please reflow the paragraph to 72 characters.

> 
> Signed-off-by: yahia ahmed <yahia.a.abdrabou@gmail.com>

A Fixes tag?

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

Please don't leave empty line between call and its error handling.

> +		device_link_del(link);
> +	return ret;
>  }
>  
>  /*
> 

-- 
 i.