From: "Madhani, Himanshu" <Himanshu.Madhani@cavium.com>
To: Max Kellermann <mk@cm4all.com>
CC: Greg KH <gregkh@linuxfoundation.org>,
        linux-scsi <linux-scsi@vger.kernel.org>,
        Dept-Eng QLA2xxx Upstream <qla2xxx-upstream@cavium.com>,
        "max.kellermann@gmail.com" <max.kellermann@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] drivers/scsi/qla2xxx: fix double free bug after firmware
 timeout
Thread-Topic: [PATCH] drivers/scsi/qla2xxx: fix double free bug after firmware
 timeout
Thread-Index: AQHTjiXzy1qsO0dDlkWHzWVZLq6UeqN1WjUA
Date: Mon, 15 Jan 2018 19:58:47 +0000
Message-ID: <13B82B86-8810-4722-97F9-EC5B862ECC76@cavium.com>
References: <151603716189.28707.13105191089826357709.stgit@rabbit.intern.cm-ag>
In-Reply-To: <151603716189.28707.13105191089826357709.stgit@rabbit.intern.cm-ag>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0D10ED0DEBA34247A4F3CF7DEC64569C@namprd07.prod.outlook.com>
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 293e8d68-8e86-4de4-a496-08d55c5263ee
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Jan 2018 19:58:47.5044
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 711e4ccf-2e9b-4bcf-a551-4094005b6194
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN4PR0701MB3629
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Return-Path: <linux-kernel-owner@vger.kernel.org>

Hi Max, 

> On Jan 15, 2018, at 9:26 AM, Max Kellermann <mk@cm4all.com> wrote:
> 
> 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)
> 
> Signed-off-by: Max Kellermann <mk@cm4all.com>
> ---
> drivers/scsi/qla2xxx/qla_init.c |    3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index b5b48ddca962..801890564e00 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -58,7 +58,6 @@ qla2x00_sp_timeout(unsigned long __data)
> 	req->outstanding_cmds[sp->handle] = NULL;
> 	iocb = &sp->u.iocb_cmd;
> 	iocb->timeout(sp);
> -	sp->free(sp);
> 	spin_unlock_irqrestore(&vha->hw->hardware_lock, flags);
> }
> 
> @@ -121,9 +120,11 @@ qla2x00_async_iocb_timeout(void *data)
> 		ea.data[1] = lio->u.logio.data[1];
> 		ea.sp = sp;
> 		qla24xx_handle_plogi_done_event(fcport->vha, &ea);
> +		sp->free(sp);
> 		break;
> 	case SRB_LOGOUT_CMD:
> 		qlt_logo_completion_handler(fcport, QLA_FUNCTION_TIMEOUT);
> +		sp->free(sp);
> 		break;
> 	case SRB_CT_PTHRU_CMD:
> 	case SRB_MB_IOCB:
> 

We have patch to prevent this double free in 4.16/scsi-queue already. 

https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git/commit/?h=4.16/scsi-queue&id=045d6ea200af794ba15515984cff63787a7fc3c0

Thanks,
- Himanshu