Skip to content

Commit

Permalink
scsi: qla2xxx: Fix double free bug after firmware timeout
Browse files Browse the repository at this point in the history
This patch is based on Max's original patch.

When the qla2xxx firmware is unavailable, eventually
qla2x00_sp_timeout() is reached, which calls the timeout function and
frees the srb_t instance.

The timeout function always resolves to qla2x00_async_iocb_timeout(),
which invokes another callback function called "done".  All of these
qla2x00_*_sp_done() callbacks also free the srb_t instance; after
returning to qla2x00_sp_timeout(), it is freed again.

The fix is to remove the "sp->free(sp)" call from qla2x00_sp_timeout()
and add it to those code paths in qla2x00_async_iocb_timeout() which
do not already free the object.

This is how it looks like with KASAN:

BUG: KASAN: use-after-free in qla2x00_sp_timeout+0x228/0x250
Read of size 8 at addr ffff88278147a590 by task swapper/2/0

Allocated by task 1502:
save_stack+0x33/0xa0
kasan_kmalloc+0xa0/0xd0
kmem_cache_alloc+0xb8/0x1c0
mempool_alloc+0xd6/0x260
qla24xx_async_gnl+0x3c5/0x1100

Freed by task 0:
save_stack+0x33/0xa0
kasan_slab_free+0x72/0xc0
kmem_cache_free+0x75/0x200
qla24xx_async_gnl_sp_done+0x556/0x9e0
qla2x00_async_iocb_timeout+0x1c7/0x420
qla2x00_sp_timeout+0x16d/0x250
call_timer_fn+0x36/0x200

The buggy address belongs to the object at ffff88278147a440
which belongs to the cache qla2xxx_srbs of size 344
The buggy address is located 336 bytes inside of
344-byte region [ffff88278147a440, ffff88278147a598)

Reported-by: Max Kellermann <mk@cm4all.com>
Signed-off-by: Quinn Tran <quinn.tran@cavium.com>
Signed-off-by: Himanshu Madhani <himanshu.madhani@cavium.com>
Cc: Max Kellermann <mk@cm4all.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
  • Loading branch information
Quinn Tran authored and Martin K. Petersen committed Feb 14, 2018
1 parent cabe92a commit eaf75d1
Showing 1 changed file with 3 additions and 20 deletions.
23 changes: 3 additions & 20 deletions drivers/scsi/qla2xxx/qla_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ qla2x00_sp_timeout(struct timer_list *t)
req->outstanding_cmds[sp->handle] = NULL;
iocb = &sp->u.iocb_cmd;
iocb->timeout(sp);
if (sp->type != SRB_ELS_DCMD)
sp->free(sp);
spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
}

Expand Down Expand Up @@ -102,7 +100,6 @@ qla2x00_async_iocb_timeout(void *data)
srb_t *sp = data;
fc_port_t *fcport = sp->fcport;
struct srb_iocb *lio = &sp->u.iocb_cmd;
struct event_arg ea;

if (fcport) {
ql_dbg(ql_dbg_disc, fcport->vha, 0x2071,
Expand All @@ -117,25 +114,13 @@ qla2x00_async_iocb_timeout(void *data)

switch (sp->type) {
case SRB_LOGIN_CMD:
if (!fcport)
break;
/* Retry as needed. */
lio->u.logio.data[0] = MBS_COMMAND_ERROR;
lio->u.logio.data[1] = lio->u.logio.flags & SRB_LOGIN_RETRIED ?
QLA_LOGIO_LOGIN_RETRIED : 0;
memset(&ea, 0, sizeof(ea));
ea.event = FCME_PLOGI_DONE;
ea.fcport = sp->fcport;
ea.data[0] = lio->u.logio.data[0];
ea.data[1] = lio->u.logio.data[1];
ea.sp = sp;
qla24xx_handle_plogi_done_event(fcport->vha, &ea);
sp->done(sp, QLA_FUNCTION_TIMEOUT);
break;
case SRB_LOGOUT_CMD:
if (!fcport)
break;
qlt_logo_completion_handler(fcport, QLA_FUNCTION_TIMEOUT);
break;
case SRB_CT_PTHRU_CMD:
case SRB_MB_IOCB:
case SRB_NACK_PLOGI:
Expand Down Expand Up @@ -235,12 +220,10 @@ static void
qla2x00_async_logout_sp_done(void *ptr, int res)
{
srb_t *sp = ptr;
struct srb_iocb *lio = &sp->u.iocb_cmd;

sp->fcport->flags &= ~(FCF_ASYNC_SENT | FCF_ASYNC_ACTIVE);
if (!test_bit(UNLOADING, &sp->vha->dpc_flags))
qla2x00_post_async_logout_done_work(sp->vha, sp->fcport,
lio->u.logio.data);
sp->fcport->login_gen++;
qlt_logo_completion_handler(sp->fcport, res);
sp->free(sp);
}

Expand Down

0 comments on commit eaf75d1

Please sign in to comment.