Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1177311AbdDYJQj (ORCPT ); Tue, 25 Apr 2017 05:16:39 -0400 Received: from mx0a-0016f401.pphosted.com ([67.231.148.174]:43145 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1177295AbdDYJQd (ORCPT ); Tue, 25 Apr 2017 05:16:33 -0400 Date: Tue, 25 Apr 2017 17:11:34 +0800 From: Jisheng Zhang To: Peter Chen CC: "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH] usb: chipidea: udc: fix NULL pointer dereference if udc_start failed Message-ID: <20170425171134.6a983841@xhacker> In-Reply-To: <20170425082948.GB873@b29397-desktop> References: <20170424123551.2465-1-jszhang@marvell.com> <20170425082948.GB873@b29397-desktop> X-Mailer: Claws Mail 3.14.1 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-04-25_04:,, signatures=0 X-Proofpoint-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=2 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1703280000 definitions=main-1704250171 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3901 Lines: 89 Hi Peter, On Tue, 25 Apr 2017 16:29:48 +0800 Peter Chen wrote: > 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] [] dma_pool_free+0xb8/0xf0 > > [ 2.118857] [] destroy_eps+0x4c/0x68 > > [ 2.124165] [] ci_hdrc_gadget_destroy+0x28/0x50 > > [ 2.130461] [] ci_hdrc_probe+0x588/0x7e8 > > [ 2.136129] [] platform_drv_probe+0x50/0xb8 > > [ 2.142066] [] driver_probe_device+0x1fc/0x2a8 > > [ 2.148270] [] __device_attach_driver+0x9c/0xf8 > > [ 2.154563] [] bus_for_each_drv+0x58/0x98 > > [ 2.160317] [] __device_attach+0xc4/0x138 > > [ 2.166072] [] device_initial_probe+0x10/0x18 > > [ 2.172185] [] bus_probe_device+0x94/0xa0 > > [ 2.177940] [] device_add+0x3f0/0x560 > > [ 2.183337] [] platform_device_add+0x180/0x240 > > [ 2.189541] [] ci_hdrc_add_device+0x440/0x4f8 > > [ 2.195654] [] ci_hdrc_usb2_probe+0x13c/0x2d8 > > [ 2.201769] [] platform_drv_probe+0x50/0xb8 > > [ 2.207705] [] driver_probe_device+0x1fc/0x2a8 > > [ 2.213910] [] __driver_attach+0xac/0xb0 > > [ 2.219575] [] bus_for_each_dev+0x60/0xa0 > > [ 2.225329] [] driver_attach+0x20/0x28 > > [ 2.230816] [] bus_add_driver+0x1d0/0x238 > > [ 2.236571] [] driver_register+0x60/0xf8 > > [ 2.242237] [] __platform_driver_register+0x44/0x50 > > [ 2.248891] [] ci_hdrc_usb2_driver_init+0x18/0x20 > > [ 2.255365] [] do_one_initcall+0x38/0x128 > > [ 2.261121] [] kernel_init_freeable+0x1ac/0x250 > > [ 2.267414] [] kernel_init+0x10/0x100 > > [ 2.272810] [] ret_from_fork+0x10/0x50 > > > > Signed-off-by: Jisheng Zhang > > --- > > 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. > I got your meaning. I'll cook v2. I don't have preference, since either one can fix the issue. Thanks for your suggestion, Jisheng