| Message ID | 20260831020451.585944-34-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Use defined 16-bits ASC/ASCQ combinations | expand |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Pre-existing issues: - [High] Use-After-Free when an asynchronous I/O completion enqueues work onto a destroyed workqueue during device removal. -- commit d0f50a340823cdbe80b900f5e21694a095dae714 Author: Damien Le Moal <dlemoal@kernel.org> scsi: storvsc: use 16-bits defined sense codes This refactors the storvsc driver to use the 16-bits sense_code field of struct scsi_sense_hdr. It replaces hard-coded additional sense codes with the enum values defined in include/scsi/scsi_sense.h to improve code clarity. > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c > index ef3cd3bc7390e..625db97d1a03e 100644 > --- a/drivers/scsi/storvsc_drv.c > +++ b/drivers/scsi/storvsc_drv.c [ ... ] > @@ -1033,7 +1033,7 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb, > case SRB_STATUS_DATA_OVERRUN: > if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID) { > /* Check for capacity change */ > - if ((asc == 0x2a) && (ascq == 0x9)) { > + if (sshdr->sense_code == CAPACITY_DATA_HAS_CHANGED) { > process_err_fn = storvsc_device_scan; > /* Retry the I/O that triggered this. */ > set_host_byte(scmnd, DID_REQUEUE); > goto do_work; [Severity: High] This isn't a bug introduced by this patch, but can this lead to a use-after-free during device removal? In storvsc_remove(), the driver destroys the workqueue before stopping the VMBus channel and draining outstanding requests: drivers/scsi/storvsc_drv.c:storvsc_remove() { ... destroy_workqueue(host_dev->handle_error_wq); scsi_remove_host(host); storvsc_dev_remove(dev); ... } If an asynchronous I/O completion indicating a capacity change arrives after this point, it will call storvsc_handle_error(), take this goto do_work path, and blindly enqueue work onto the destroyed workqueue: drivers/scsi/storvsc_drv.c:storvsc_handle_error() { ... do_work: ... queue_work(host_dev->handle_error_wq, &wrk->work); } > } [ ... ]
diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c index ef3cd3bc7390..625db97d1a03 100644 --- a/drivers/scsi/storvsc_drv.c +++ b/drivers/scsi/storvsc_drv.c @@ -1016,7 +1016,7 @@ static int storvsc_channel_init(struct hv_device *device, bool is_fc) static void storvsc_handle_error(struct vmscsi_request *vm_srb, struct scsi_cmnd *scmnd, struct Scsi_Host *host, - u8 asc, u8 ascq) + struct scsi_sense_hdr *sshdr) { struct storvsc_scan_work *wrk; void (*process_err_fn)(struct work_struct *work); @@ -1033,7 +1033,7 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb, case SRB_STATUS_DATA_OVERRUN: if (vm_srb->srb_status & SRB_STATUS_AUTOSENSE_VALID) { /* Check for capacity change */ - if ((asc == 0x2a) && (ascq == 0x9)) { + if (sshdr->sense_code == CAPACITY_DATA_HAS_CHANGED) { process_err_fn = storvsc_device_scan; /* Retry the I/O that triggered this. */ set_host_byte(scmnd, DID_REQUEUE); @@ -1049,8 +1049,12 @@ static void storvsc_handle_error(struct vmscsi_request *vm_srb, * want scsi_report_sense() to output a message * that a sysadmin wouldn't know what to do with. */ - if ((asc == 0x3f) && (ascq != 0x03) && - (ascq != 0x0e)) { + if (scsi_sense_asc(sshdr) == + ASC_TARGET_OPERATING_CONDITIONS_HAVE_CHANGED && + sshdr->sense_code != + INQUIRY_DATA_HAS_CHANGED && + sshdr->sense_code != + REPORTED_LUNS_DATA_HAS_CHANGED) { process_err_fn = storvsc_device_scan; set_host_byte(scmnd, DID_REQUEUE); goto do_work; @@ -1141,8 +1145,7 @@ static void storvsc_command_completion(struct storvsc_cmd_request *cmd_request, } if (vm_srb->srb_status != SRB_STATUS_SUCCESS) { - storvsc_handle_error(vm_srb, scmnd, host, sense_hdr.asc, - sense_hdr.ascq); + storvsc_handle_error(vm_srb, scmnd, host, &sense_hdr); /* * The Windows driver set data_transfer_length on * SRB_STATUS_DATA_OVERRUN. On other errors, this value
Refactor the storvsc driver to use the 16-bits sense_code field of struct scsi_sense_hdr and replace all hard-coded additional sense codes and additional sense code qualifiers with the enum values defined in include/scsi/scsi_sense.h. This helps with code clarity as the sense codes being processed are easier to test and self-documented. No functional change intended. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/scsi/storvsc_drv.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)