Date: Tue, 25 Apr 2017 16:29:48 +0800
From: Peter Chen <peter.chen@nxp.com>
To: Jisheng Zhang <jszhang@marvell.com>
CC: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] usb: chipidea: udc: fix NULL pointer dereference if
 udc_start failed
Message-ID: <20170425082948.GB873@b29397-desktop>
References: <20170424123551.2465-1-jszhang@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20170424123551.2465-1-jszhang@marvell.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Apr 2017 08:29:57.4676
 (UTC)
X-MS-Exchange-CrossTenant-Id: 5afe0b00-7697-4969-b663-5eab37d5f47e
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=5afe0b00-7697-4969-b663-5eab37d5f47e;Ip=[192.88.168.50];Helo=[tx30smr01.am.freescale.net]
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0301MB1195
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3573
Lines: 83

On Mon, Apr 24, 2017 at 12:35:51PM +0000, Jisheng Zhang wrote:
> Fix below NULL pointer dereference. we set ci->roles[CI_ROLE_GADGET]
> too early in ci_hdrc_gadget_init(), if udc_start() fails due to some
> reason, the ci->roles[CI_ROLE_GADGET] check in  ci_hdrc_gadget_destroy
> can't protect us.
> 
> We fix this issue by only setting ci->roles[CI_ROLE_GADGET] if
> udc_start() succeed.
> 
> [    1.398550] Unable to handle kernel NULL pointer dereference at
> virtual address 00000000
> ...
> [    1.448600] PC is at dma_pool_free+0xb8/0xf0
> [    1.453012] LR is at dma_pool_free+0x28/0xf0
> [    2.113369] [<ffffff80081817d8>] dma_pool_free+0xb8/0xf0
> [    2.118857] [<ffffff800841209c>] destroy_eps+0x4c/0x68
> [    2.124165] [<ffffff8008413770>] ci_hdrc_gadget_destroy+0x28/0x50
> [    2.130461] [<ffffff800840fa30>] ci_hdrc_probe+0x588/0x7e8
> [    2.136129] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
> [    2.142066] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
> [    2.148270] [<ffffff800837f68c>] __device_attach_driver+0x9c/0xf8
> [    2.154563] [<ffffff800837d570>] bus_for_each_drv+0x58/0x98
> [    2.160317] [<ffffff800837f174>] __device_attach+0xc4/0x138
> [    2.166072] [<ffffff800837f738>] device_initial_probe+0x10/0x18
> [    2.172185] [<ffffff800837e58c>] bus_probe_device+0x94/0xa0
> [    2.177940] [<ffffff800837c560>] device_add+0x3f0/0x560
> [    2.183337] [<ffffff8008380d20>] platform_device_add+0x180/0x240
> [    2.189541] [<ffffff800840f0e8>] ci_hdrc_add_device+0x440/0x4f8
> [    2.195654] [<ffffff8008414194>] ci_hdrc_usb2_probe+0x13c/0x2d8
> [    2.201769] [<ffffff8008380fb8>] platform_drv_probe+0x50/0xb8
> [    2.207705] [<ffffff800837f494>] driver_probe_device+0x1fc/0x2a8
> [    2.213910] [<ffffff800837f5ec>] __driver_attach+0xac/0xb0
> [    2.219575] [<ffffff800837d4b0>] bus_for_each_dev+0x60/0xa0
> [    2.225329] [<ffffff800837ec80>] driver_attach+0x20/0x28
> [    2.230816] [<ffffff800837e880>] bus_add_driver+0x1d0/0x238
> [    2.236571] [<ffffff800837fdb0>] driver_register+0x60/0xf8
> [    2.242237] [<ffffff8008380ef4>] __platform_driver_register+0x44/0x50
> [    2.248891] [<ffffff80086fd440>] ci_hdrc_usb2_driver_init+0x18/0x20
> [    2.255365] [<ffffff8008082950>] do_one_initcall+0x38/0x128
> [    2.261121] [<ffffff80086e0d00>] kernel_init_freeable+0x1ac/0x250
> [    2.267414] [<ffffff800852f0b8>] kernel_init+0x10/0x100
> [    2.272810] [<ffffff8008082680>] ret_from_fork+0x10/0x50
> 
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  drivers/usb/chipidea/udc.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index f88e9157fad0..60a786c87c06 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1984,6 +1984,7 @@ static void udc_id_switch_for_host(struct ci_hdrc *ci)
>  int ci_hdrc_gadget_init(struct ci_hdrc *ci)
>  {
>  	struct ci_role_driver *rdrv;
> +	int ret;
>  
>  	if (!hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC))
>  		return -ENXIO;
> @@ -1996,7 +1997,10 @@ int ci_hdrc_gadget_init(struct ci_hdrc *ci)
>  	rdrv->stop	= udc_id_switch_for_host;
>  	rdrv->irq	= udc_irq;
>  	rdrv->name	= "gadget";
> -	ci->roles[CI_ROLE_GADGET] = rdrv;
>  
> -	return udc_start(ci);
> +	ret = udc_start(ci);
> +	if (!ret)
> +		ci->roles[CI_ROLE_GADGET] = rdrv;
> +
> +	return ret;
>  }
> -- 

Thanks for fixing it. In fact, we'd better return failure if
ret && ret != -ENXIO at probe, it stands for initialization
for host or gadget has failed.

-- 

Best Regards,
Peter Chen