[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@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=9b195c4f412ea5c4e56a
Tested-by: syzbot+9b195c4f412ea5c4e56a@xxxxxxxxxxxxxxxxxxxxxxxxx
Signed-off-by: Jie Wang <jie.wang@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxx/

 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