| Message ID | 20240229093756.129324-1-rand.sec96@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ssb: Fix potential NULL pointer dereference in ssb_device_uevent | expand |
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
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>
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.
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,
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(-)