* Re: usb: atm: cxacru: fix NULL pointer dereference on uninitialized atm_dev
2026-08-31 14:40 [PATCH] usb: atm: cxacru: fix NULL pointer dereference on uninitialized atm_dev syzbot
@ 2026-09-02 8:42 ` netdev-bot+sashiko
0 siblings, 0 replies; 2+ messages in thread
From: netdev-bot+sashiko @ 2026-09-02 8:42 UTC (permalink / raw)
To: syzbot
Cc: syzkaller-bugs, jie.wang, 3chas3, accessrunner-general, gregkh,
linux-atm-general, linux-usb, netdev, linux-kernel, syzbot, kuba
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
^ permalink raw reply [flat|nested] 2+ messages in thread