Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Critical] Missing eh_host_reset_handler causes SCSI EH to free commands while the host is active, resulting in DMA memory corruption and Use-After-Free. --
commit c1b61825f071ba8c8020eead463039fdb142c7b0 Author: Nguyen Ngoc Thang <[email protected]> scsi: virtio_scsi: bound EH timer resets to avoid unkillable hang This commit limits the number of times virtscsi_eh_timed_out() can reset the command timer to 5, returning SCSI_EH_NOT_HANDLED afterward. It also bounds the wait time in virtscsi_tmf() when handling aborts and resets to prevent infinite hangs if the transport breaks. Closes: https://syzkaller.appspot.com/bug?extid=53706c567afab5131044 > diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c > index 35731b18c519e..b4f20c4877182 100644 > --- a/drivers/scsi/virtio_scsi.c > +++ b/drivers/scsi/virtio_scsi.c [ ... ] > @@ -783,13 +818,19 @@ static void virtscsi_commit_rqs(struct Scsi_Host > *shost, u16 hwq) > } > > /* > - * The host guarantees to respond to each command, although I/O > - * latencies might be higher than on bare metal. Reset the timer > - * unconditionally to give the host a chance to perform EH. > + * The host normally answers every command, so reset the timer and keep > + * waiting. But if the transport is broken (e.g. bus mastering was turned > + * off), no completion can ever arrive: give up after a few resets so SCSI > + * EH fails the command instead of blocking its submitter forever. > */ > static enum scsi_timeout_action virtscsi_eh_timed_out(struct scsi_cmnd > *scmnd) > { > - return SCSI_EH_RESET_TIMER; > + struct virtio_scsi_cmd *cmd = scsi_cmd_priv(scmnd); > + > + if (++cmd->eh_resets < VIRTSCSI_EH_RESET_LIMIT) > + return SCSI_EH_RESET_TIMER; > + > + return SCSI_EH_NOT_HANDLED; [Severity: Critical] Does returning SCSI_EH_NOT_HANDLED here allow SCSI EH to run and eventually free the command without ensuring the host has dropped its DMA references? If the virtio host experiences a severe delay and eventually completes the timed-out commands, could it DMA the response directly into memory that has already been freed by SCSI EH and potentially reallocated to new requests? If this happens, wouldn't it result in a use-after-free and DMA memory corruption when virtscsi_complete_cmd() runs? For example: virtscsi_complete_cmd() { struct virtio_scsi_cmd *cmd = buf; struct scsi_cmnd *sc = cmd->sc; struct virtio_scsi_cmd_resp *resp = &cmd->resp.cmd; ... scsi_done(sc); } Here the dangling pointer from the virtqueue would be dereferenced after the command has been freed by SCSI EH, potentially followed by a kernel panic when scsi_done() is called on the corrupted scsi_cmnd pointer. > } > > static const struct scsi_host_template virtscsi_host_template = { [Severity: Critical] Does the host template lack an eh_host_reset_handler? Without this, SCSI EH cannot safely stop the virtio device before freeing the timed-out commands. If SCSI EH attempts aborts and device resets which also time out, it will give up, offline the device, and flush (free) the commands. When the host later resumes and processes the virtqueue, could it overwrite the freed memory? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
