[PATCH] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[PATCH] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

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

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,
-- 
2.34.1

Re: [PATCH] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Jonas Gorski]

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

Re: [PATCH] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Michael Büsch]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

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>


-- 
Michael Büsch
https://bues.ch/

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Kalle Valo]

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.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
> Acked-by: Michael Büsch <m@bues.ch>

Failed to apply:

error: sha1 information is lacking or useless (drivers/ssb/main.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Applying: ssb: Fix potential NULL pointer dereference in ssb_device_uevent
Patch failed at 0001 ssb: Fix potential NULL pointer dereference in ssb_device_uevent

Patch set to Changes Requested.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20240229093756.129324-1-rand.sec96@gmail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

[PATCH v2] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Rand Deeb]

Hi Kalle,

It seems there's been a mix-up in applying the patch. The previous patch
was intended for the linux-5.10.y branch, not the master branch. I
appreciate your attention to detail.

The following patch has been tailored for the master branch and should
resolve the issue properly. Thank you for your understanding.

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

diff --git a/drivers/ssb/main.c b/drivers/ssb/main.c
index b9934b9c2d70..070a99a4180c 100644
--- a/drivers/ssb/main.c
+++ b/drivers/ssb/main.c
@@ -341,11 +341,13 @@ static int ssb_bus_match(struct device *dev, struct device_driver *drv)
 
 static int ssb_device_uevent(const struct device *dev, struct kobj_uevent_env *env)
 {
-	const struct ssb_device *ssb_dev = dev_to_ssb_dev(dev);
+	const 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,
-- 
2.34.1

Re: [PATCH v2] ssb: Fix potential NULL pointer dereference in ssb_device_uevent

[Kalle Valo]

Rand Deeb <rand.sec96@gmail.com> writes:

> Hi Kalle,
>
> It seems there's been a mix-up in applying the patch. The previous patch
> was intended for the linux-5.10.y branch, not the master branch. I
> appreciate your attention to detail.
>
> The following patch has been tailored for the master branch and should
> resolve the issue properly. Thank you for your understanding.

This text should be below '---' line so that it's not included to the
git history and you should restore the original commit message. Please
submit v3 with the correct commit message and please also read the wiki
link below.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches