| 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
| ||
|
Message-ID: <50168B9A.5080605@evidence.eu.com>
Date: Mon, 30 Jul 2012 15:26:50 +0200
From: Claudio Scordino <claudio@...dence.eu.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: ok@...ecdesign.ee, linux-usb@...r.kernel.org,
Linux Kernel <linux-kernel@...r.kernel.org>,
bruno <bruno@...dence.eu.com>
Subject: Re: [PATCH] isp1362-hcd.c: usb message always saved in case of underrun
Il 20/07/2012 17:15, Greg KH ha scritto:
> On Fri, Jul 20, 2012 at 12:26:10PM +0200, Claudio Scordino wrote:
>> Il 20/07/2012 00:58, Greg KH ha scritto:
>>> On Wed, Jul 18, 2012 at 10:53:09AM +0200, Claudio Scordino wrote:
>>>> Il 06/07/2012 19:41, Greg KH ha scritto:
>>>>> On Wed, Jun 27, 2012 at 06:01:39PM +0200, Claudio Scordino wrote:
>>>>>> Hi Olav,
>>>>>>
>>>>>> please find below a patch for the isp1362-hcd.c driver to always
>>>>>> save the message in case of underrun. More information is provided
>>>>>> inside the patch comment. Let us know if you need any further
>>>>>> information.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Claudio
>>>>>>
>>>>>>
>>>>>> Subject: isp1362-hcd.c: usb message always saved in case of underrun
>>>>>> From: Bruno Morelli<bruno@...dence.eu.com>
>>>>>>
>>>>>> The usb message must be saved also in case the USB endpoint is not a
>>>>>> control endpoint (i.e., "endpoint 0"), otherwise in some circumstances
>>>>>> we don't have a payload in case of error.
>>>>>>
>>>>>> The patch has been created by tracing with usbmon the different error
>>>>>> messages generated by this driver with respect to the ehci-hcd driver.
>>>>>>
>>>>>> Signed-off-by: Bruno Morelli<bruno@...dence.eu.com>
>>>>>> Signed-off-by: Claudio Scordino<claudio@...dence.eu.com>
>>>>>> Tested-by: Bruno Morelli<bruno@...dence.eu.com>
>>>>>> ---
>>>>>> drivers/usb/host/isp1362-hcd.c | 11 ++++++-----
>>>>>> 1 files changed, 6 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c
>>>>>> index 2ed112d..61bf1b2 100644
>>>>>> --- a/drivers/usb/host/isp1362-hcd.c
>>>>>> +++ b/drivers/usb/host/isp1362-hcd.c
>>>>>> @@ -543,13 +543,14 @@ static void postproc_ep(struct isp1362_hcd *isp1362_hcd, struct isp1362_ep *ep)
>>>>>> usb_pipein(urb->pipe) ? "IN" : "OUT", ep->nextpid,
>>>>>> short_ok ? "" : "not_",
>>>>>> PTD_GET_COUNT(ptd), ep->maxpacket, len);
>>>>>> + /* save the data underrun error code for later and
>>>>>> + * proceed with the status stage
>>>>>> + */
>>>>>> + urb->actual_length += PTD_GET_COUNT(ptd);
>>>>>> + BUG_ON(urb->actual_length>
>>>>>> + urb->transfer_buffer_length);
>>>>>
>>>>> Please NEVER crash the machine in a driver like this, it's bad and can
>>>>> cause problems. Yes, I know you are just moving it from the lines
>>>>> below:
>>>>>
>>>>>> if (usb_pipecontrol(urb->pipe)) {
>>>>>> ep->nextpid = USB_PID_ACK;
>>>>>> - /* save the data underrun error code for later and
>>>>>> - * proceed with the status stage
>>>>>> - */
>>>>>> - urb->actual_length += PTD_GET_COUNT(ptd);
>>>>>> - BUG_ON(urb->actual_length> urb->transfer_buffer_length);
>>>>>
>>>>> But really, it should not be in the driver at all. Please remove it, at
>>>>> the most, do a WARN_ON() so that someone can see the problem and at
>>>>> least report it.
>>>>>
>>>>> Actually, what is this checking? How can someone recover from it? Who
>>>>> is this check for? The developer of this driver? Another driver?
>>>>> Hardware developer? End user? Who would be able to fix the problem if
>>>>> this happens?
>>>>>
>>>>> As it is, I can't take it, sorry.
>>>>
>>>>
>>>> Hi Greg.
>>>>
>>>> I understand. As you have already said, this driver is full of BUG_ON
>>>> statements.
>>>>
>>>> So, we can shift just the assignment as in the patch below, to have a
>>>> correct behavior, leaving the BUG_ON where it already is. Then, we may
>>>> propose a different patch to change BUG_ONs to WARN_ONs.
>>>
>>> Your updated patch is much better, thanks.
>>>
>>> But it doesn't apply to my tree right now. Can you please refresh it
>>> against the usb-next tree and resend it?
>>
>> Actually, I did.
>>
>> So, this means that I'm using the wrong tree...
>>
>> I'm using the "usb-next" branch available on your tree at
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
>>
>> Is this the wrong one ?
>
> That is the correct one. It didn't work for me, so try refreshing your
> patch and resending it.
Hi Greg.
I've just refreshed the patch by cloning the repo from scratch and
creating the patch against the "usb-next" branch.
You can find the patch below, but the content is still the same.
Can you please check it again ? Is it possible that you have local
changes not yet committed ?
Many thanks,
Claudio
Subject: isp1362-hcd.c: usb message always saved in case of underrun
From: Bruno Morelli <bruno@...dence.eu.com>
The usb message must be saved also in case the USB endpoint is not a
control endpoint (i.e., "endpoint 0"), otherwise in some circumstances
we don't have a payload in case of error.
The patch has been created by tracing with usbmon the different error
messages generated by this driver with respect to the ehci-hcd driver.
Signed-off-by: Bruno Morelli <bruno@...dence.eu.com>
Signed-off-by: Claudio Scordino <claudio@...dence.eu.com>
Tested-by: Bruno Morelli <bruno@...dence.eu.com>
---
drivers/usb/host/isp1362-hcd.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/host/isp1362-hcd.c b/drivers/usb/host/isp1362-hcd.c
index 2ed112d..2563263 100644
--- a/drivers/usb/host/isp1362-hcd.c
+++ b/drivers/usb/host/isp1362-hcd.c
@@ -543,12 +543,12 @@ static void postproc_ep(struct isp1362_hcd *isp1362_hcd, struct isp1362_ep *ep)
usb_pipein(urb->pipe) ? "IN" : "OUT", ep->nextpid,
short_ok ? "" : "not_",
PTD_GET_COUNT(ptd), ep->maxpacket, len);
+ /* save the data underrun error code for later and
+ * proceed with the status stage
+ */
+ urb->actual_length += PTD_GET_COUNT(ptd);
if (usb_pipecontrol(urb->pipe)) {
ep->nextpid = USB_PID_ACK;
- /* save the data underrun error code for later and
- * proceed with the status stage
- */
- urb->actual_length += PTD_GET_COUNT(ptd);
BUG_ON(urb->actual_length > urb->transfer_buffer_length);
if (urb->status == -EINPROGRESS)
--
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists