linux-kernel - Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1455646709.22112.412.camel@localhost.localdomain>
Date:	Tue, 16 Feb 2016 13:18:29 -0500
From:	Ewan Milne <emilne@...hat.com>
To:	Insu Yun <wuninsu@...il.com>
Cc:	nagalakshmi.nandigama@...gotech.com,
	praveen.krishnamoorthy@...gotech.com,
	sreekanth.reddy@...gotech.com, abhijit.mahajan@...gotech.com,
	MPT-FusionLinux.pdl@...gotech.com, linux-scsi@...r.kernel.org,
	linux-kernel@...r.kernel.org, taesoo@...ech.edu,
	yeongjin.jang@...ech.edu, insu@...ech.edu, changwoo@...ech.edu
Subject: Re: [PATCH] fusion-mptbase: handle failed allocation for workqueue

On Mon, 2016-02-15 at 21:50 -0500, Insu Yun wrote:
> the failure of ioc->reset_work_q is checked,
> but not ioc->fw_event_q.
> 
> Signed-off-by: Insu Yun <wuninsu@...il.com>
> ---
>  drivers/message/fusion/mptbase.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/message/fusion/mptbase.c b/drivers/message/fusion/mptbase.c
> index 5dcc031..d4907a1 100644
> --- a/drivers/message/fusion/mptbase.c
> +++ b/drivers/message/fusion/mptbase.c
> @@ -1996,6 +1996,13 @@ mpt_attach(struct pci_dev *pdev, const struct pci_device_id *id)
>  	snprintf(ioc->fw_event_q_name, MPT_KOBJ_NAME_LEN, "mpt/%d", ioc->id);
>  	ioc->fw_event_q = create_singlethread_workqueue(ioc->fw_event_q_name);
>  
> +	if (!ioc->fw_event_q) {
> +		destroy_workqueue(ioc->reset_work_q);
> +		pci_release_selected_regions(pdev, ioc->bars);
> +		kfree(ioc);
> +		return -ENOMEM;
> +	}
> +
>  	if ((r = mpt_do_ioc_recovery(ioc, MPT_HOSTEVENT_IOC_BRINGUP,
>  	    CAN_SLEEP)) != 0){
>  		printk(MYIOC_s_ERR_FMT "didn't initialize properly! (%d)\n",

This does not look correct to me.  The error path for the call to
mpt_do_ioc_recovery() after create_singlethread_workqueue() for
ioc->fw_event_q does other cleanup, including:

                list_del(&ioc->list);
                if (ioc->alt_ioc)
                        ioc->alt_ioc->alt_ioc = NULL;
                iounmap(ioc->memmap);

and

                kfree(ioc);
                pci_set_drvdata(pdev, NULL);

Here I think you are kfree()ing ioc while it is still on the &ioc_list,
which will cause a crash.

Note to Avago:  this code could use a symbolic return code identifier:

                if (r != -5)
                        pci_release_selected_regions(pdev, ioc->bars);

In general, with sequential allocation of resources like this, error
handling can be performed using a series of goto's to labels at the
end of the function that release the resources in reverse order.  This
avoids the duplication of code within the function, and reduces the
chance for errors when the function is later modified.  See init_sd
in drivers/scsi/sd.c for an example.

Reviewed-by: Ewan D. Milne <emilne@...hat.com>