Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755112AbcDDKxa (ORCPT <rfc822;w@1wt.eu>);
	Mon, 4 Apr 2016 06:53:30 -0400
Received: from mga01.intel.com ([192.55.52.88]:46058 "EHLO mga01.intel.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1753293AbcDDKx3 (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
	Mon, 4 Apr 2016 06:53:29 -0400
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.24,440,1455004800"; 
   d="asc'?scan'208";a="951172653"
From: Felipe Balbi <felipe.balbi@linux.intel.com>
To: John Youn <John.Youn@synopsys.com>, John Keeping <john@metanate.com>,
        John Youn <John.Youn@synopsys.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] usb: dwc2: gadget: avoid null dereference on incomplete transfer
In-Reply-To: <56FC5707.80601@synopsys.com>
References: <1457983214-4317-1-git-send-email-john@metanate.com> <87k2kkhzk1.fsf@intel.com> <56FC5707.80601@synopsys.com>
User-Agent: Notmuch/0.21+96~g9bbc54b (http://notmuchmail.org) Emacs/25.0.90.3 (x86_64-pc-linux-gnu)
Date: Mon, 04 Apr 2016 13:51:34 +0300
Message-ID: <871t6l7ikp.fsf@intel.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha1; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3299
Lines: 97

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable


Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 3/30/2016 6:22 AM, Felipe Balbi wrote:
>>=20
>> Hi,
>>=20
>> John Keeping <john@metanate.com> writes:
>>> Setting up a gadget with the uac2 function results in:
>>>
>>>   Unable to handle kernel NULL pointer dereference at virtual address 0=
0000058
>>>   ...
>>>   PC is at dwc2_hsotg_irq+0x7f0/0x908
>>>   LR is at dwc2_hsotg_irq+0x4c/0x908
>>>   Backtrace:
>>>   [<c03cd5fc>] (dwc2_hsotg_irq) from [<c00814fc>] (handle_irq_event_per=
cpu+0x130/0x3ec)
>>>   [<c00813cc>] (handle_irq_event_percpu) from [<c0081800>] (handle_irq_=
event+0x48/0x6c)
>>>
>>> In all other loops we already skip endpoints that are null, so do so
>>> here as well.
>>>
>>> Signed-off-by: John Keeping <john@metanate.com>
>>> ---
>>>  drivers/usb/dwc2/gadget.c | 8 ++++++--
>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 0abf73c..df43ec0 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -2606,7 +2606,9 @@ irq_retry:
>>>  		for (idx =3D 1; idx < hsotg->num_of_eps; idx++) {
>>>  			hs_ep =3D hsotg->eps_in[idx];
>>>=20=20
>>> -			if (!hs_ep->isochronous || hs_ep->has_correct_parity)
>>> +			if (!hs_ep ||
>>> +			    !hs_ep->isochronous ||
>>> +			    hs_ep->has_correct_parity)
>>=20
>> this is fine (even though choice of where to break line is a bit odd),
>> but I have a question about how the rest of the code works (a bit
>> off-topic, sorry)
>>=20
>>>  				continue;
>>>=20=20
>>>  			epctl_reg =3D DIEPCTL(idx);
>>=20
>> So, this means that the first ISO endpoint without correct parity will
>> be used. Isn't this a bit fragile ? What happens when you use a device
>> with several different interfaces using several different endpoints ?
>>=20
>> Isn't there a register where we can check which physical endpoint
>> generated the IRQ ? Seems like you really wanna check what:
>>=20
>
> We discussed this back when the patch was first submitted and
> determined it should work fine like this. I don't remember exactly why
> though.
>
> But this ISOC parity stuff is a workaround and we have a series of
> patches to correctly set up ISOC allowing us to remove it. We're doing
> some final tests before we send them.

fair enough, thanks

=2D-=20
balbi

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJXAkc3AAoJEIaOsuA1yqREbngQALNEOlVZk96r65MA5+1bARVD
814AQNcR9NOYyfsDEDpelRtkAxwQgvMt7xzhM1202iQCjOdKSpXpzjhbb/Ctnr5F
0wmc3tdPn1ALRXz8tJjpuACBvINM2+9hrfejmvQOlQIxtOBc4/hiBAltx/vZKRFG
WwuMY6V6r+37iIbPs41Mk7rQ21EIVWtKyeAPDvniqL6KCJJ27QlkJ6P407XojVLk
b89J/nsPXMd1fDazlW9riAUn/+HncRk59Z1xy7kpILUzL/X/0uyWVopl4XASyqPL
l5CMRodrVB4/0SAfb4ep999TcCoJTISgjubgSSqVQzVNQe39gAcq4oUYRgTfcQ8E
s6ILRWEuN/rKst9lLmivZJRUqB+5Nb8jVvUfpGR9nu6bYUNBgwGzKe2AVKbPGhXA
zUwl8xb4iiLEsUJwJAHUpuysntzBR8eO95urdDZDz1F5ntuPd7PzI77TO3Ief0Lp
Je4e6RVQioCzkoOmh/RQVVHYNN/kbPh2Z2A8EnwSUmD1Mve51RdJol7sqBD2/Opc
1LDu8r6qSuCf8lcrLKVGrS0NF662AO8Huf6eIDRbu9tMJgsDGRgnRFJrL4zcqOJw
RuyFMPCHadlVHQm9daNlD5aSM+sfFa3r/DA2gW6DjBo4/CcTMjvr/V7HXfYSEtc3
hYAaP73/717braTymbC7
=sXqJ
-----END PGP SIGNATURE-----
--=-=-=--