[Bug 101891] mvsas prep failed, NULL pointer dereference in mvs_slot_task_free+0x5/0x1f0 [mvsas]

[bugzilla-daemon@bugzilla.kernel.org]

https://bugzilla.kernel.org/show_bug.cgi?id=101891

--- Comment #4 from Dāvis <davispuh@gmail.com> ---
(In reply to Dāvis from comment #3)
> I narrowed it down to this section of mvs_abort_task function
> (drivers/scsi/mvsas/mv_sas.c)
> 
> 	} else if (task->task_proto & SAS_PROTOCOL_SATA ||
> 		task->task_proto & SAS_PROTOCOL_STP) {
> 		if (SAS_SATA_DEV == dev->dev_type) {
> 			struct mvs_slot_info *slot = task->lldd_task;
> 			u32 slot_idx = (u32)(slot - mvi->slot_info);
> 			mv_dprintk("mvs_abort_task() mvi=%p task=%p "
> 				   "slot=%p slot_idx=x%x\n",
> 				   mvi, task, slot, slot_idx);
> 			task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> 			mvs_slot_task_free(mvi, task, slot, slot_idx);
> 			rc = TMF_RESP_FUNC_COMPLETE;
> 			goto out;
> 		}
> 
> 	}
> 
> 
> Basically this line "u32 slot_idx = (u32)(slot - mvi->slot_info)".
> I think (slot - mvi->slot_info) returns 0x10 and that's why
> (there's no "mvs_abort_task()" in journal so it crashes before that.
> 

Sorry for being idiot, that line doesn't cause any pointer
dereference and neither does previous line. It's just so obvious,
compiler reordered instructions so that mvs_slot_task_free is executed
before mv_dprintk is called and that's why it's not in journal.
Even as title I wrote NULL pointer dereference in mvs_slot_task_free
and that's exactly where had to look.

So anyway when in mvs_task_prep and if pci_pool_alloc fails then
task->lldd_task is NULL as can see

    task->lldd_task = NULL;
    slot->n_elem = n_elem;
    slot->slot_tag = tag;

    slot->buf = pci_pool_alloc(mvi->dma_pool, GFP_ATOMIC, &slot->buf_dma);
    if (!slot->buf)
        goto err_out_tag;

then later it's aborted with mvs_abort_task and there mvs_slot_task_free
is called with (slot = task->lldd_task) which is NULL and in
mvs_slot_task_free
{
    if (!slot->task)
        return;

happens this NULL pointer dereference because slot is NULL.

There's 2 ways to fix this, either check if slot is NULL before calling 
mvs_slot_task_free or just inside it check it.

I went for second option as it seems easier and won't have to always
check before calling.

Here's a patch, haven't tested it yet but I think it will fix this
and it's compiling right now so I'll let know once I'll have tested it.

diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 454536c..9c78074 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -887,6 +887,8 @@ static void mvs_slot_free(struct mvs_info *mvi, u32
rx_desc)
 static void mvs_slot_task_free(struct mvs_info *mvi, struct sas_task *task,
                          struct mvs_slot_info *slot, u32 slot_idx)
 {
+       if (!slot)
+               return;
        if (!slot->task)
                return;
        if (!sas_protocol_ata(task->task_proto))

-- 
You are receiving this mail because:
You are watching the assignee of the bug.--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html