[PATCH] usb: atm: cxacru: fix NULL pointer dereference on uninitialized atm_dev

[PATCH] usb: atm: cxacru: fix NULL pointer dereference on uninitialized atm_dev

[syzbot]

From: "Wang, Jie" <jie.wang@intel.com>

The cxacru driver uses the usbatm framework, which defers the ATM device
registration to a kernel thread (usbatm_do_heavy_init()) but immediately
returns success from the USB probe. Because the probe returns success, the
driver core creates sysfs attribute files (like adsl_state and
adsl_config), making them accessible to userspace before the ATM device is
fully initialized.

If a user writes to these sysfs files before the initialization is complete
or if it fails, the driver attempts to send a command to the device. If the
command fails, the error handling path calls atm_err(), which
unconditionally dereferences instance->usbatm->atm_dev. Since atm_dev is
NULL, this leads to a NULL pointer dereference:

Oops: general protection fault, probably for non-canonical address
0xdffffc0000000002: 0000 [#1] SMP KASAN NOPTI
KASAN: null-ptr-deref in range [0x0000000000000010-0x0000000000000017]
RIP: 0010:adsl_state_store+0x5cc/0x770 drivers/usb/atm/cxacru.c:359
Call Trace:
 <TASK>
 kernfs_fop_write_iter+0x3a4/0x540 fs/kernfs/file.c:345
 new_sync_write fs/read_write.c:595 [inline]
 vfs_write+0x612/0xba0 fs/read_write.c:687
 ksys_write+0x150/0x270 fs/read_write.c:739
 do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
 do_syscall_64+0x174/0x580 arch/x86/entry/syscall_64.c:94
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
 </TASK>

To fix this, add checks for instance->usbatm->atm_dev == NULL in
adsl_state_store() and adsl_config_store(). This prevents the functions
from proceeding and dereferencing the uninitialized atm_dev, consistent
with how mac_address_show() handles the same race condition.

Fixes: e605c30977bb ("USB: atm: cxacru: convert to use dev_groups")
Assisted-by: Gemini:gemini-3.6-flash Gemini:gemini-3.1-pro-preview syzbot
Reported-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a
Link: https://syzkaller.appspot.com/ai_job?id=b6352d63-d03a-4e87-92a4-b334c3ebcf97
Signed-off-by: "Wang, Jie" <jie.wang@intel.com>

---
diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 429ac20a8..dee15f7b7 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -347,7 +347,7 @@ static ssize_t adsl_state_store(struct device *dev,
 		return -EINVAL;
 	ret = 0;
 
-	if (instance == NULL)
+	if (instance == NULL || instance->usbatm->atm_dev == NULL)
 		return -ENODEV;
 
 	if (mutex_lock_interruptible(&instance->adsl_state_serialize))
@@ -444,7 +444,7 @@ static ssize_t adsl_config_store(struct device *dev,
 	if (!capable(CAP_NET_ADMIN))
 		return -EACCES;
 
-	if (instance == NULL)
+	if (instance == NULL || instance->usbatm->atm_dev == NULL)
 		return -ENODEV;
 
 	pos = 0;


base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
See https://goo.gle/syzbot-ai-patches for information about AI-generated patches.
The person who has signed off on the patch is responsible for
addressing comments.
syzbot engineers can be reached at syzkaller@googlegroups.com.

Re: usb: atm: cxacru: fix NULL pointer dereference on uninitialized atm_dev

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: the new `instance->usbatm->atm_dev == NULL`
  guards in adsl_state_store() (drivers/usb/atm/cxacru.c:350) and…

--

usb: atm: cxacru: fix NULL pointer dereference on uninitialized atm_dev

The commit adds a NULL test on instance->usbatm->atm_dev at the entry of
adsl_state_store() and adsl_config_store() in drivers/usb/atm/cxacru.c, so
that a sysfs write arriving before usbatm_do_heavy_init() has registered the
ATM device returns -ENODEV instead of dereferencing atm_dev through
atm_err().

Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a
Link: https://syzkaller.appspot.com/ai_job?id=b6352d63-d03a-4e87-92a4-b334c3ebcf97

> diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
> index 429ac20a8999f..dee15f7b70bf6 100644
> --- a/drivers/usb/atm/cxacru.c
> +++ b/drivers/usb/atm/cxacru.c
> @@ -347,7 +347,7 @@ static ssize_t adsl_state_store(struct device *dev,
>  		return -EINVAL;
>  	ret = 0;
>  
> -	if (instance == NULL)
> +	if (instance == NULL || instance->usbatm->atm_dev == NULL)
>  		return -ENODEV;
>  
>  	if (mutex_lock_interruptible(&instance->adsl_state_serialize))

[Severity: High]
Is this check taken early enough to still be valid at the point where
atm_dev is actually dereferenced?

The test reads instance->usbatm->atm_dev once at handler entry, but the
dereference happens later in atm_err(), which expands through atm_printk()
in drivers/usb/atm/usbatm.h:

  #define atm_printk(level, instance, format, arg...)	\
	printk(level "ATM dev %d: " format ,		\
	(instance)->atm_dev->number , ## arg)

Between the check and that dereference the handler sleeps in
mutex_lock_interruptible(&instance->adsl_state_serialize), then in
cxacru_cm() (which takes cm_serialize and performs a USB control transfer),
and on the "restart" path also in msleep(1500).

Meanwhile usbatm_atm_init() in drivers/usb/atm/usbatm.c publishes atm_dev
before calling atm_start, and clears plus frees it if atm_start fails:

  instance->atm_dev = atm_dev;
  ...
  if (instance->driver->atm_start && ((ret = instance->driver->atm_start(instance, atm_dev)) < 0)) {
	atm_err(instance, "%s: atm_start failed: %d!\n", __func__, ret);
	goto fail;
  }
  ...
 fail:
	instance->atm_dev = NULL;
	atm_dev_deregister(atm_dev); /* usbatm_atm_dev_close will eventually be called */

So a non-NULL atm_dev does not mean initialization completed, and the only
failing leg of cxacru_atm_start() runs before it takes the mutex that
adsl_state_store() uses:

  ret = cxacru_cm(instance, CM_REQUEST_CARD_GET_MAC_ADDRESS, NULL, 0,
		  atm_dev->esi, sizeof(atm_dev->esi));
  if (ret < 0) {
	atm_err(usbatm_instance, "cxacru_atm_start: CARD_GET_MAC_ADDRESS returned %d\n", ret);
	return ret;
  }

  /* start ADSL */
  mutex_lock(&instance->adsl_state_serialize);

With that ordering, can the exact oops quoted in the commit message still
happen?  A writer passes the new check while atm_dev is published but
atm_start has not finished, sleeps in cxacru_cm(), the heavy-init kthread
takes the fail path above, and the writer then evaluates:

  atm_err(instance->usbatm, "change adsl state:"
	  " CHIP_ADSL_LINE_STOP returned %d\n", ret);

which is the reported crash site.  The CHIP_ADSL_LINE_START error leg is
identical.  If the compiler had already loaded the old pointer, is this a
use-after-free instead, given atm_dev_deregister() drops the last reference?

There is a second window of the same shape in usbatm_usb_disconnect():

  if (instance->atm_dev) {
	atm_dev_deregister(instance->atm_dev);
	instance->atm_dev = NULL;
  }

Would it be more robust to drop the atm_dev dereference from these paths
altogether, for example by using usb_err()/dev_err() on
instance->usb_intf->dev, which stays valid for the duration of the sysfs
write?  Alternatively, re-validating and latching atm_dev under a lock that
usbatm_atm_init()'s failure path also takes, holding a reference on the
atm_dev, or publishing instance->atm_dev only after initialization has fully
succeeded would close the window rather than narrow it.

Related to the same window: adsl_state_store() calls cxacru_poll_status()
after dropping adsl_state_serialize, and that function caches the pointer
with no check of its own:

  struct usbatm_data *usbatm = instance->usbatm;
  struct atm_dev *atm_dev = usbatm->atm_dev;

and then uses it via atm_warn(), atm_dev->link_rate and
atm_dev_signal_change().  Can that path fault too once atm_dev has been
cleared behind the writer's back?

> @@ -444,7 +444,7 @@ static ssize_t adsl_config_store(struct device *dev,
>  	if (!capable(CAP_NET_ADMIN))
>  		return -EACCES;
>  
> -	if (instance == NULL)
> +	if (instance == NULL || instance->usbatm->atm_dev == NULL)
>  		return -ENODEV;
>  
>  	pos = 0;

[Severity: High]
The same check-then-use concern applies here, and this handler holds no
mutex at all before the sleeping cxacru_cm() transfers inside the
while (pos < len) loop.  Both the failure and the success legs dereference
atm_dev after that sleep:

  ret = cxacru_cm(instance, CM_REQUEST_CARD_DATA_SET,
		  (u8 *) data, 4 + num * 8, NULL, 0);
  if (ret < 0) {
	atm_err(instance->usbatm,
		"set card data returned %d\n", ret);
	return -EIO;
  }
  ...
  atm_info(instance->usbatm, "config%s\n", log);

Since atm_info() expands to the same (instance)->atm_dev->number, does this
mean no command failure is even required to hit the dereference here once
usbatm_atm_init() or usbatm_usb_disconnect() has cleared atm_dev?

Would it make sense to also drop the atm_err()/atm_info() uses in this
function in favour of usb_err()/usb_info() on the USB interface?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/f3ef0a67-79d7-47da-be6e-7e82d92e68fe%40mail.kernel.org

[PATCH v2] usb: atm: cxacru: fix NULL deref of atm_dev on sysfs writes

[Jie Wang]

The adsl_state and adsl_config sysfs attributes are created via
dev_groups, so the driver core exposes them as soon as ->probe()
returns. At that point instance->atm_dev can still be NULL:
usbatm_heavy_init() only waits for the heavy-init kthread to start, not
to finish, and instance->atm_dev is assigned later, in usbatm_atm_init().

A write to either attribute in that window passes the existing
"instance == NULL" check and reaches atm_err()/atm_info() on the error
path (and, for adsl_config, on the success path). Both expand to
instance->atm_dev->number and dereference the NULL atm_dev.

Fix this by logging with usb_err()/usb_info() instead. They reference the
USB interface device, which stays valid for the whole write, and
usbatm_atm_init() already logs this way before atm_dev exists.

adsl_state_store() also calls cxacru_poll_status() directly, which
dereferences atm_dev via atm_dev_signal_change() and atm_dev->link_rate.
Return early from the poll when atm_dev is not yet set.

Fixes: e605c30977bb ("USB: atm: cxacru: convert to use dev_groups")
Reported-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a
Tested-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
Signed-off-by: Jie Wang <jie.wang@intel.com>
---
Changes since v1 [1]:
 - v1 gated on ->atm_dev at handler entry, but that check is an
   unsynchronized TOCTOU; remove the ->atm_dev dereference from these
   paths instead.

[1] https://lore.kernel.org/all/f3ef0a67-79d7-47da-be6e-7e82d92e68fe@mail.kernel.org/

 drivers/usb/atm/cxacru.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 75d87c31e81f4..4322553f2902c 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -356,7 +356,7 @@ static ssize_t adsl_state_store(struct device *dev,
 	if (!strcmp(str_cmd, "stop") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_STOP, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(instance->usbatm, "change adsl state:"
+			usb_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_STOP returned %d\n", ret);
 
 			ret = -EIO;
@@ -376,7 +376,7 @@ static ssize_t adsl_state_store(struct device *dev,
 	if (!strcmp(str_cmd, "start") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_START, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(instance->usbatm, "change adsl state:"
+			usb_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_START returned %d\n", ret);
 
 			ret = -EIO;
@@ -481,7 +481,7 @@ static ssize_t adsl_config_store(struct device *dev,
 			ret = cxacru_cm(instance, CM_REQUEST_CARD_DATA_SET,
 				(u8 *) data, 4 + num * 8, NULL, 0);
 			if (ret < 0) {
-				atm_err(instance->usbatm,
+				usb_err(instance->usbatm,
 					"set card data returned %d\n", ret);
 				return -EIO;
 			}
@@ -490,7 +490,7 @@ static ssize_t adsl_config_store(struct device *dev,
 				snprintf(log + tmp*12, 13, " %02x=%08x",
 					le32_to_cpu(data[tmp * 2 + 1]),
 					le32_to_cpu(data[tmp * 2 + 2]));
-			atm_info(instance->usbatm, "config%s\n", log);
+			usb_info(instance->usbatm, "config%s\n", log);
 			num = 0;
 		}
 	}
@@ -827,6 +827,12 @@ static void cxacru_poll_status(struct work_struct *work)
 	int keep_polling = 1;
 	int ret;
 
+	/* adsl_state_store() calls this directly, before the heavy-init thread
+	 * has set up atm_dev; bail out rather than dereference NULL below.
+	 */
+	if (!atm_dev)
+		return;
+
 	ret = cxacru_cm_get_array(instance, CM_REQUEST_CARD_INFO_GET, buf, CXINF_MAX);
 	if (ret < 0) {
 		if (ret != -ESHUTDOWN)
-- 
2.43.0

Re: [PATCH v2] usb: atm: cxacru: fix NULL deref of atm_dev on sysfs writes

[Greg Kroah-Hartman]

On Tue, Sep 08, 2026 at 03:02:03PM +0000, Jie Wang wrote:
> The adsl_state and adsl_config sysfs attributes are created via
> dev_groups, so the driver core exposes them as soon as ->probe()
> returns. At that point instance->atm_dev can still be NULL:
> usbatm_heavy_init() only waits for the heavy-init kthread to start, not
> to finish, and instance->atm_dev is assigned later, in usbatm_atm_init().
> 
> A write to either attribute in that window passes the existing
> "instance == NULL" check and reaches atm_err()/atm_info() on the error
> path (and, for adsl_config, on the success path). Both expand to
> instance->atm_dev->number and dereference the NULL atm_dev.
> 
> Fix this by logging with usb_err()/usb_info() instead. They reference the
> USB interface device, which stays valid for the whole write, and
> usbatm_atm_init() already logs this way before atm_dev exists.
> 
> adsl_state_store() also calls cxacru_poll_status() directly, which
> dereferences atm_dev via atm_dev_signal_change() and atm_dev->link_rate.
> Return early from the poll when atm_dev is not yet set.
> 
> Fixes: e605c30977bb ("USB: atm: cxacru: convert to use dev_groups")
> Reported-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a
> Tested-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
> Signed-off-by: Jie Wang <jie.wang@intel.com>

Did you forget an assisted-by: tag?


> ---
> Changes since v1 [1]:
>  - v1 gated on ->atm_dev at handler entry, but that check is an
>    unsynchronized TOCTOU; remove the ->atm_dev dereference from these
>    paths instead.

Sashiko still has some objections:
	https://sashiko.dev/#/patchset/20260908150203.2119844-1-jie.wang@intel.com

[PATCH v3] usb: atm: cxacru: fix NULL deref of atm_dev on sysfs writes

[Jie Wang]

The adsl_state and adsl_config sysfs attributes are created via
dev_groups, so the driver core exposes them as soon as ->probe()
returns. At that point instance->atm_dev can still be NULL:
usbatm_heavy_init() waits only for the heavy-init kthread to start, and
atm_dev is assigned later, in usbatm_atm_init(). A write in that window
passes the "instance == NULL" check and then dereferences the NULL
atm_dev: atm_err()/atm_info() expand to
instance->usbatm->atm_dev->number, and a poll/start request also calls
cxacru_poll_status() directly, which touches atm_dev via
atm_dev_signal_change() and atm_dev->link_rate.

Log via usb_err()/usb_info() instead; they use the USB interface
device, which stays valid for the whole write. Reject a poll/start
request with -ENODEV before poll_state is advanced to CXPOLL_POLLING,
so that cxacru_atm_start() still starts polling once atm_dev is ready.

Fixes: e605c30977bb ("USB: atm: cxacru: convert to use dev_groups")
Assisted-by: Gemini:gemini-3.6-flash Gemini:gemini-3.1-pro-preview syzbot
Reported-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a
Tested-by: syzbot+9b195c4f412ea5c4e56a@syzkaller.appspotmail.com
Signed-off-by: Jie Wang <jie.wang@intel.com>
---
v3:
 - Guard in adsl_state_store() instead of cxacru_poll_status(); the v2
   guard left poll_state wedged at CXPOLL_POLLING with no work queued, so
   polling never restarted (Sashiko).
 - Restore the Assisted-by tag.

v2:
 - Drop the ->atm_dev entry gate (an unsynchronized TOCTOU); remove the
   ->atm_dev dereference from the logging paths instead.

v1: https://lore.kernel.org/all/f3ef0a67-79d7-47da-be6e-7e82d92e68fe@mail.kernel.org/
v2: https://lore.kernel.org/all/20260908150203.2119844-1-jie.wang@intel.com/

 drivers/usb/atm/cxacru.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
index 636f7886fc260..501a51b27e466 100644
--- a/drivers/usb/atm/cxacru.c
+++ b/drivers/usb/atm/cxacru.c
@@ -356,7 +356,7 @@ static ssize_t adsl_state_store(struct device *dev,
 	if (!strcmp(str_cmd, "stop") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_STOP, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(instance->usbatm, "change adsl state:"
+			usb_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_STOP returned %d\n", ret);
 
 			ret = -EIO;
@@ -376,7 +376,7 @@ static ssize_t adsl_state_store(struct device *dev,
 	if (!strcmp(str_cmd, "start") || !strcmp(str_cmd, "restart")) {
 		ret = cxacru_cm(instance, CM_REQUEST_CHIP_ADSL_LINE_START, NULL, 0, NULL, 0);
 		if (ret < 0) {
-			atm_err(instance->usbatm, "change adsl state:"
+			usb_err(instance->usbatm, "change adsl state:"
 				" CHIP_ADSL_LINE_START returned %d\n", ret);
 
 			ret = -EIO;
@@ -396,6 +396,15 @@ static ssize_t adsl_state_store(struct device *dev,
 		poll = -1;
 	}
 
+	/* cxacru_poll_status() below dereferences atm_dev, which may not be
+	 * set up yet; reject before poll_state is advanced so that
+	 * cxacru_atm_start() can still start polling once it is.
+	 */
+	if (poll == CXPOLL_POLLING && !instance->usbatm->atm_dev) {
+		ret = -ENODEV;
+		poll = -1;
+	}
+
 	if (poll == CXPOLL_POLLING) {
 		mutex_lock(&instance->poll_state_serialize);
 		switch (instance->poll_state) {
@@ -481,7 +490,7 @@ static ssize_t adsl_config_store(struct device *dev,
 			ret = cxacru_cm(instance, CM_REQUEST_CARD_DATA_SET,
 				(u8 *) data, 4 + num * 8, NULL, 0);
 			if (ret < 0) {
-				atm_err(instance->usbatm,
+				usb_err(instance->usbatm,
 					"set card data returned %d\n", ret);
 				return -EIO;
 			}
@@ -490,7 +499,7 @@ static ssize_t adsl_config_store(struct device *dev,
 				snprintf(log + tmp*12, 13, " %02x=%08x",
 					le32_to_cpu(data[tmp * 2 + 1]),
 					le32_to_cpu(data[tmp * 2 + 2]));
-			atm_info(instance->usbatm, "config%s\n", log);
+			usb_info(instance->usbatm, "config%s\n", log);
 			num = 0;
 		}
 	}
-- 
2.43.0