Re: [PATCH RFC 1/1] leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] Incorrect teardown order in device detach and error paths leads to Use-After-Free via the workqueue.
- [High] Bypassing `scsi_alloc_request` leaves critical `scsi_cmnd` fields uninitialized.
- [High] Missing serialization for the multi-step ENE hardware command sequence allows concurrent workers to interleave and corrupt hardware state.
- [Low] Undefined macro `kzalloc_obj` is used for memory allocation.
- [Low] `blk_rq_map_kern` is called with insufficient arguments.
- [Low] The driver prints a success log message on probe/attach.
- [Low] The commit message violates the LED subsystem capitalization and prefix rules.
--

commit 241d70bc8fb77ca0865693a679d9c3bdc51ad240
Author: Liang Haowen <nbg2974@gmail.com>

leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

This commit adds a SCSI driver for ASUS ROG external NVMe enclosures to
support Aura LEDs. The driver exposes each LED as a multicolor LED class
device and communicates with the ENE controller using vendor SCSI commands.

> leds: add ASUS Aura SCSI driver for ROG NVMe enclosures

[Severity: Low]
Does this commit message subject follow the LED subsystem guidelines?
The guidelines specify that the description after the subsystem prefix should
be capitalized and include the driver name, such as:
leds: asus-aura-scsi: Add ASUS Aura SCSI driver for ROG NVMe enclosures

> diff --git a/drivers/leds/leds-asus-aura-scsi.c b/drivers/leds/leds-asus-aura-scsi.c
> new file mode 100644
> index 0000000000000..c913501ba96a2
> --- /dev/null
> +++ b/drivers/leds/leds-asus-aura-scsi.c
[ ... ]
> +static int ene_write(struct scsi_device *sdev, u16 reg,
> +		     const void *data, u8 arg_count)
> +{
> +	struct request *rq;
> +	struct scsi_cmnd *scmd;
> +	u8 cdb[ENE_CDB_LEN];
> +	int ret;
> +
> +	ene_build_cdb(cdb, reg, arg_count);
> +
> +	rq = blk_mq_alloc_request(sdev->request_queue, REQ_OP_DRV_OUT, 0);
> +	if (IS_ERR(rq))
> +		return PTR_ERR(rq);

[Severity: High]
Does bypassing scsi_alloc_request() here leave critical scsi_cmnd fields
uninitialized? Using blk_mq_alloc_request() directly skips
scsi_initialize_rq(), which means PDU fields like jiffies_at_alloc, rcu,
and retries remain uninitialized. When blk_execute_rq() runs, this might
trigger false timeouts or undefined behavior in the SCSI midlayer.

> +
> +	scmd = blk_mq_rq_to_pdu(rq);
> +	scmd->cmd_len = ENE_CDB_LEN;
> +	memcpy(scmd->cmnd, cdb, ENE_CDB_LEN);
> +
> +	if (arg_count) {
> +		ret = blk_rq_map_kern(rq,
> +				      (void *)data, arg_count, GFP_KERNEL);

[Severity: Low]
Can this blk_rq_map_kern() call compile? It seems to be missing the required
first parameter for the request queue (struct request_queue *q), passing
only 4 arguments instead of the mandatory 5.

> +		if (ret)
> +			goto out;
> +	}
[ ... ]
> +static void asus_aura_led_work(struct work_struct *work)
> +{
> +	struct asus_aura_led *led =
> +		container_of(work, struct asus_aura_led, work);
> +	struct scsi_device *sdev = led->zone->sdev;
> +	u8 apply = ENE_APPLY;
> +	u8 save = ENE_SAVE;
> +	u8 mode = ENE_MODE_STATIC;
> +	int ret;
> +
> +	if (!scsi_device_online(sdev))
> +		return;
> +
> +	/* Mode first: without it the device ignores the whole sequence. */
> +	ret = ene_write(sdev, ENE_REG_MODE, &mode, 1);
> +	if (ret)
> +		goto err;
> +
> +	ret = ene_write(sdev, ENE_REG_COLORS + led->index * ENE_RGB_LEN,
> +			led->rgb, ENE_RGB_LEN);

[Severity: High]
Could concurrent sysfs writes to different LEDs on the same device corrupt
the hardware state here? Because there are 4 LEDs on the same device,
multiple workers could interleave their ene_write() block requests to the
same endpoint without a shared mutex, potentially breaking the strict
multi-step sequence required by the ENE controller.

> +	if (ret)
> +		goto err;
[ ... ]
> +static int asus_aura_attach(struct scsi_device *sdev)
> +{
> +	struct asus_aura_zone *zone;
> +	int i, ret;
> +
> +	if (strncmp(sdev->vendor, ARION_INQ_VENDOR, strlen(ARION_INQ_VENDOR)) ||
> +	    strncmp(sdev->model, ARION_INQ_MODEL, strlen(ARION_INQ_MODEL)))
> +		return SCSI_DH_DEV_UNSUPP;
> +
> +	zone = kzalloc_obj(*zone, GFP_KERNEL);

[Severity: Low]
Is kzalloc_obj() a valid macro in the kernel API? It appears this will cause
a build failure, and should likely be standard kzalloc(sizeof(*zone),
GFP_KERNEL) instead.

> +	if (!zone)
> +		return SCSI_DH_NOMEM;
> +	zone->sdev = sdev;
> +
> +	for (i = 0; i < ARION_NUM_LEDS; i++) {
> +		ret = asus_aura_register_led(zone, i);
> +		if (ret)
> +			goto err_free;
> +	}
> +
> +	sdev->handler_data = zone;
> +	sdev_printk(KERN_INFO, sdev,
> +		    "asus_aura: %d per-LED multicolor LEDs registered\n",
> +		    ARION_NUM_LEDS);

[Severity: Low]
Is it okay to print this success log message on probe? The LED subsystem
guidelines specify that drivers should avoid logging success messages, and
only log errors or warnings.

> +	return SCSI_DH_OK;
> +
> +err_free:
> +	while (i--) {
> +		cancel_work_sync(&zone->leds[i].work);
> +		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);

[Severity: High]
Could this teardown order introduce a use-after-free risk? Unregistering the
LED sets its brightness to 0, which invokes asus_aura_set() and queues a
new work item onto the system workqueue. Since cancel_work_sync() has
already been called, a dangling work item might remain when kfree(zone)
is executed shortly after.

> +		kfree(zone->leds[i].mc_cdev.led_cdev.name);
> +	}
> +	kfree(zone);
> +	return SCSI_DH_NOMEM;
> +}
> +
> +static void asus_aura_detach(struct scsi_device *sdev)
> +{
> +	struct asus_aura_zone *zone = sdev->handler_data;
> +	int i;
> +
> +	if (!zone)
> +		return;
> +	for (i = 0; i < ARION_NUM_LEDS; i++) {
> +		cancel_work_sync(&zone->leds[i].work);
> +		led_classdev_multicolor_unregister(&zone->leds[i].mc_cdev);

[Severity: High]
Similar to the error path in asus_aura_attach(), does unregistering the LED
after cancel_work_sync() leave a dangling work item? The unregister call
might queue new work via asus_aura_set(), leading to a use-after-free when
the memory is subsequently freed.

> +		kfree(zone->leds[i].mc_cdev.led_cdev.name);
> +	}
> +	kfree(zone);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/202609012200.RFC1.lhw@gmail.com?part=1