asd_get_response_tasklet() takes the empty data buffer index out of the done list status block the sequencer wrote, with the same arithmetic escb_tasklet_complete() uses on its own copy: int edb_id = ((resp_sb->flags & 0x70) >> 4)-1; The extracted field is three bits, so the subtraction yields -1 through 6, and the function indexes seq->edb_arr[] with it without looking: edb = asd_ha->seq.edb_arr[edb_id + escb->edb_index]; r = edb->vaddr; asd_assign_edbs2escbs() gives the first ESCB edb_index 0, so for that one edb_id == -1 reads seq->edb_arr[-1], one pointer before the kmalloc'd array, and the next line dereferences it. asd_invalidate_edb() at the end of the function would index escb->eb[] and seq->edb_arr[] with the same value. Reject an index outside [0, ASD_EDBS_PER_SCB) and return, and set ts->stat on the way out, because returning without touching it reports the command as a success. asd_task_tasklet_complete() sets ts->resp and ts->stat before it calls this function: case TC_SSP_RESP: case TC_ATA_RESP: ts->resp = SAS_TASK_COMPLETE; ts->stat = SAS_PROTO_RESPONSE; asd_get_response_tasklet(ascb, dl); SAS_PROTO_RESPONSE is a promise that this function will go on to put the real status there, from the response IU for SSP or the ending FIS for STP. Returning early breaks that promise and leaves the promise in place. For SSP, sas_end_task() starts from DID_OK and SAS_SAM_STAT_GOOD, and its SAS_PROTO_RESPONSE case only prints a notice, so the command completes as a success. For STP, sas_ata_task_done() takes SAS_PROTO_RESPONSE as "the ending FIS has the status" and copies ATA_RESP_FIS_SIZE out of ts->buf, which this function never filled, then derives err_mask from it. So the bail reports SAS_INTERRUPTED, which is what this driver already uses for a response that arrived but could not be used -- TI_BREAK, TI_PROTO_ERR, TI_NAK, TI_ACK_NAK_TO, TF_SMP_XMIT_RCV_ERR and TC_ATA_R_ERR_RECV all map to it. sas_end_task() turns it into DID_NO_CONNECT, and sas_to_ata_err() into AC_ERR_ATA_BUS, so neither path mistakes it for a completed command. The lookup failure above it had the same problem and gets the same line. Nothing changes on the path where the index is in range. Whether the sequencer can really report zero in that field is not something the tree answers. It is firmware loaded with request_firmware(), status_block[] is an opaque four bytes in aic94xx_sas.h whose layout is described only by a struct declared locally in this function and again in aic94xx_tmf.c, and there is no Documentation/ file or MAINTAINERS entry for this driver. This is not a claim of a reachable out-of-bounds access. It makes the driver consistent with itself: asd_bytes_dmaed_tasklet() in aic94xx_scb.c clamps a device-supplied length a few lines after indexing seq->edb_arr[] with a device-supplied index and no bound of its own. As with the preceding patch, a response dropped here leaves its ESCB uninvalidated and so not reposted, since the repost happens inside asd_invalidate_edb(). The alternative is to index the array with a value the driver cannot place. Fixes: 2908d778ab3e ("[SCSI] aic94xx: new driver") Signed-off-by: Yehyeong Lee --- drivers/scsi/aic94xx/aic94xx_task.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/scsi/aic94xx/aic94xx_task.c b/drivers/scsi/aic94xx/aic94xx_task.c index b26a468ddc981..bb5094ca5de15 100644 --- a/drivers/scsi/aic94xx/aic94xx_task.c +++ b/drivers/scsi/aic94xx/aic94xx_task.c @@ -166,6 +166,14 @@ static void asd_get_response_tasklet(struct asd_ascb *ascb, if (!escb) { ASD_DPRINTK("Uh-oh! No escb for this dl?!\n"); + ts->stat = SAS_INTERRUPTED; + return; + } + + if (edb_id < 0 || edb_id >= ASD_EDBS_PER_SCB) { + ASD_DPRINTK("edb_id is %d! resp_sb->flags is 0x%x\n", + edb_id, resp_sb->flags); + ts->stat = SAS_INTERRUPTED; return; } -- 2.43.0