# Re: \[PATCH v3\] usb: atm: cxacru: fix NULL deref of atm\_dev on sysfs writes - From: Jakub Kicinski \ - Date: 2026-09-26 02:35:18 - List: netdev - Page: https://ratatoskr.run/netdev/2026/09/17645546 - Thread: https://ratatoskr.run/linux-usb/2026/08/17485259/t.md ```text This is an AI-generated review of your patch. The human sending this email has considered the AI review valid, or at least plausible. --- usb: atm: cxacru: fix NULL deref of atm_dev on sysfs writes This commit changes the adsl_state and adsl_config sysfs store handlers to log with usb_err()/usb_info() instead of atm_err()/atm_info(). A write that arrives before usbatm_atm_init() has set atm_dev therefore no longer dereferences a NULL atm_dev. It also rejects poll/start requests with -ENODEV while atm_dev is NULL, before poll_state is advanced. Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a > 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 [ ... ] > @@ -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; > + } [Severity: High] Is this check enough to keep cxacru_poll_status() away from an invalid atm_dev? The read is not locked, and nothing keeps atm_dev valid after the check passes. usbatm_atm_init() publishes atm_dev before atm_start() runs. If atm_start() fails, it clears atm_dev and frees it: drivers/usb/atm/usbatm.c:usbatm_atm_init() { ... 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 */ return ret; } The device controls whether cxacru_atm_start() fails on the MAC address read. That failure returns before adsl_state_serialize is taken and before poll_state is looked at: drivers/usb/atm/cxacru.c:cxacru_atm_start() { ... 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; } ... } A "poll" write sends no CM request before this check. If one arrives while CARD_GET_MAC_ADDRESS is in flight, it sees a non-NULL atm_dev, moves poll_state from CXPOLL_STOPPED to CXPOLL_POLLING, and calls cxacru_poll_status() directly. "start" and "restart" can do the same. cxacru_poll_status() then goes one of two ways. (a) It saves the still-valid pointer in a local: struct atm_dev *atm_dev = usbatm->atm_dev; It then blocks on cm_serialize behind the MAC request in cxacru_cm_get_array(). By the time it writes through atm_dev_signal_change() or atm_dev->link_rate, atm_dev_deregister() has freed the atm_dev. (b) It loads NULL and dereferences it in atm_dev_signal_change(), or in atm_warn()/atm_info(), which expand to usbatm->atm_dev->number. cxacru_atm_start() never resets poll_state on that failure, and the heavy-init thread exits with the interface still bound. So poll_work keeps rescheduling itself: if (keep_polling) schedule_delayed_work(&instance->poll_work, round_jiffies_relative(POLL_INTERVAL*HZ)); After that, every poll error or line status change dereferences the NULL atm_dev, with no further writes needed. The v2 notes dropped an earlier atm_dev gate as "an unsynchronized TOCTOU". Does this gate have the same problem? Would closing it need real synchronization, for example gating on heavy init completion, or serializing the atm_start() failure path with the sysfs writers? > + > if (poll == CXPOLL_POLLING) { > mutex_lock(&instance->poll_state_serialize); > switch (instance->poll_state) { [Severity: Medium] This is a pre-existing issue, not one introduced by this patch. mac_address_show() uses the same unlocked check and then reads the pointer again: if (instance == NULL || instance->usbatm->atm_dev == NULL) return -ENODEV; return sprintf(buf, "%pM\n", instance->usbatm->atm_dev->esi); Suppose the usbatm_atm_init() failure path clears and frees atm_dev after the reader loads the pointer but before vsprintf copies the six bytes. Can this hand freed slab memory to userspace? The attribute has no CAP_NET_ADMIN check. A NULL atm_dev here only gives a small pointer, which vsprintf prints as "(efault)". So the concern is the use-after-free read, not an oops. The window has no sleeping point, so it is narrow. [ ... ] > @@ -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); [Severity: Medium] This is also a pre-existing issue, not one introduced by this patch. Since the dev_groups conversion in e605c30977bb, adsl_config_store() and adsl_state_store() can send CM requests (CARD_DATA_SET, CHIP_ADSL_LINE_STOP, CHIP_ADSL_LINE_START) through cxacru_cm() while the heavy-init thread is still running cxacru_upload_firmware(). At that point the chip may still be in boot ROM or part way through the FW_WRITE_MEM sequence. cxacru_upload_firmware() later clears halt on the command endpoints, and a URB submitted from sysfs may still be queued there: drivers/usb/atm/cxacru.c:cxacru_upload_firmware() { ... usb_clear_halt(usb_dev, usb_sndbulkpipe(usb_dev, CXACRU_EP_CMD)); usb_clear_halt(usb_dev, usb_rcvbulkpipe(usb_dev, CXACRU_EP_CMD)); ... } Is a CARD_DATA_SET config accepted before the firmware boots silently dropped once the new firmware starts? Should these writers also wait for heavy init to finish? > if (ret < 0) { > - atm_err(instance->usbatm, > + usb_err(instance->usbatm, > "set card data returned %d\n", ret); > return -EIO; > } [Severity: Medium] This is another pre-existing issue, not one introduced by this patch, but it relates to the adsl_config format. adsl_config_store() parses both fields as hex: ret = sscanf(buf + pos, "%x=%x%n", &index, &value, &tmp); cxacru.rst also documents the format as hexadecimal. The converter that cxacru.rst recommends, cxacru-cf.py in Documentation/networking/device_drivers/atm/, prints the value in decimal: sys.stdout.write("{0:x}={1}".format(i, struct.unpack("