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

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

[ ... ]

[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?

[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.

[ ... ]

[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?

[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("<I", buf)[0]))

A value of 10 comes out as "0=10", and the driver reads that as 0x10.
Should the script use {1:x} for the value?

[ ... ]
-- 
pw-bot: cr