diff mbox series

ssb: Fix potential NULL pointer dereference in ssb_device_uevent

Message ID 20240229093756.129324-1-rand.sec96@gmail.com
State New
Headers
Series ssb: Fix potential NULL pointer dereference in ssb_device_uevent |

Commit Message

Rand Deeb Feb. 29, 2024, 9:37 a.m. UTC 
  The ssb_device_uevent function first attempts to convert the 'dev' pointer
to 'struct ssb_device *'. However, it mistakenly dereferences 'dev' before
performing the NULL check, potentially leading to a NULL pointer
dereference if 'dev' is NULL.

To fix this issue, this patch moves the NULL check before dereferencing the
'dev' pointer, ensuring that the pointer is valid before attempting to use
it.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
---
 drivers/ssb/main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jonas Gorski Feb. 29, 2024, 1:40 p.m. UTC | #1 
Hi,

On Thu, 29 Feb 2024 at 10:38, Rand Deeb <rand.sec96@gmail.com> wrote:
>
> The ssb_device_uevent function first attempts to convert the 'dev' pointer
> to 'struct ssb_device *'. However, it mistakenly dereferences 'dev' before
> performing the NULL check, potentially leading to a NULL pointer
> dereference if 'dev' is NULL.
>
> To fix this issue, this patch moves the NULL check before dereferencing the
> 'dev' pointer, ensuring that the pointer is valid before attempting to use
> it.

Might be worth pointing out that dev_to_ssb_dev() does dereference
dev, in contrast to most (dev_)to_*_dev() helpers that just calculate
a new pointer from an offset via container_of(), and thus are a-okay
with NULL pointers (but I think this would be UB), or even explicitly
return NULL if the passed dev is NULL.

Though I wonder if dev can even be NULL at this point, or if the NULL
check is actually bogus and could be dropped.

AFAICT the caller of this function would be dev_uevent(), and it does it here:

        /* have the bus specific function add its stuff */
        if (dev->bus && dev->bus->uevent) {
                retval = dev->bus->uevent(dev, env);

which can only be possible if dev is non-NULL.

I can't really tell if uevent_show() would also call this function,
but even that one dereferences dev before calling uevent().

So from a first glance I would think dev is guaranteed to be non-NULL.

> (snip)

Best Regards,
Jonas
Michael Büsch Feb. 29, 2024, 6:08 p.m. UTC | #2 
On Thu, 29 Feb 2024 12:37:56 +0300
Rand Deeb <rand.sec96@gmail.com> wrote:

>  static int ssb_device_uevent(struct device *dev, struct kobj_uevent_env *env)
>  {
> -	struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
> +	struct ssb_device *ssb_dev;
>  
>  	if (!dev)
>  		return -ENODEV;
>  
> +	ssb_dev = dev_to_ssb_dev(dev);
> +
>  	return add_uevent_var(env,
>  			     "MODALIAS=ssb:v%04Xid%04Xrev%02X",
>  			     ssb_dev->id.vendor, ssb_dev->id.coreid,

Good catch.
Acked-by: Michael Büsch <m@bues.ch>
diff mbox series

Patch

diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
index 9e54bc7eec66..74f549557a01 100644
--- a/drivers/ssb/main.c
+++ b/drivers/ssb/main.c
@@ -340,11 +340,13 @@  static int ssb_bus_match(struct device *dev, struct device_driver *drv)
 
 static int ssb_device_uevent(struct device *dev, struct kobj_uevent_env *env)
 {
-	struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
+	struct ssb_device *ssb_dev;
 
 	if (!dev)
 		return -ENODEV;
 
+	ssb_dev = dev_to_ssb_dev(dev);
+
 	return add_uevent_var(env,
 			     "MODALIAS=ssb:v%04Xid%04Xrev%02X",
 			     ssb_dev->id.vendor, ssb_dev->id.coreid,