[PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[Shuah Khan]

From: Shuah Khan <shuah@kernel.org>

Fix vep_free_request() to return when usb_ep and usb_request are null
instead of calling WARN_ON.

Signed-off-by: Shuah Khan <shuah@kernel.org>
---
 drivers/usb/usbip/vudc_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 1634d8698e15..bfc8218e3fb6 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -297,7 +297,7 @@ static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct vrequest *req;
 
-	if (WARN_ON(!_ep || !_req))
+	if (!_ep || !_req)
 		return;
 
 	req = to_vrequest(_req);
-- 
2.17.1

[PATCH] usbip: Fix vhci_urb_enqueue() URB null transfer buffer error path

[Shuah Khan]

From: Shuah Khan <shuah@kernel.org>

Fix vhci_urb_enqueue() to print error and return error instead of
failing with WARN_ON.

Signed-off-by: Shuah Khan <shuah@kernel.org>
---
 drivers/usb/usbip/vhci_hcd.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 1e592ec94ba4..849ebfde87b5 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -702,8 +702,10 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
 	}
 	vdev = &vhci_hcd->vdev[portnum-1];
 
-	/* patch to usb_sg_init() is in 2.5.60 */
-	BUG_ON(!urb->transfer_buffer && urb->transfer_buffer_length);
+	if (!urb->transfer_buffer && urb->transfer_buffer_length) {
+		dev_err(dev, "Null URB transfer buffer\n");
+		return -EINVAL;
+	}
 
 	spin_lock_irqsave(&vhci->lock, flags);
 
-- 
2.17.1

Re: [PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[Greg KH]

On Fri, Jan 18, 2019 at 02:29:30PM -0700, Shuah Khan wrote:
> From: Shuah Khan <shuah@kernel.org>
> 
> Fix vep_free_request() to return when usb_ep and usb_request are null
> instead of calling WARN_ON.
> 
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
>  drivers/usb/usbip/vudc_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> index 1634d8698e15..bfc8218e3fb6 100644
> --- a/drivers/usb/usbip/vudc_dev.c
> +++ b/drivers/usb/usbip/vudc_dev.c
> @@ -297,7 +297,7 @@ static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
>  {
>  	struct vrequest *req;
>  
> -	if (WARN_ON(!_ep || !_req))
> +	if (!_ep || !_req)

It's impossible for _ep to be NULL in this callback (see
usb_ep_free_request() for where this is called from to prove that), so I
don't think you need to check that.  It's almost impossible for _req to
be NULL, so you might as well leave that check in.

thanks,

greg k-h

Re: [PATCH] usbip: Fix vhci_urb_enqueue() URB null transfer buffer error path

[Greg KH]

On Fri, Jan 18, 2019 at 02:29:31PM -0700, Shuah Khan wrote:
> From: Shuah Khan <shuah@kernel.org>
> 
> Fix vhci_urb_enqueue() to print error and return error instead of
> failing with WARN_ON.
> 
> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 1e592ec94ba4..849ebfde87b5 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -702,8 +702,10 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  	}
>  	vdev = &vhci_hcd->vdev[portnum-1];
>  
> -	/* patch to usb_sg_init() is in 2.5.60 */
> -	BUG_ON(!urb->transfer_buffer && urb->transfer_buffer_length);
> +	if (!urb->transfer_buffer && urb->transfer_buffer_length) {
> +		dev_err(dev, "Null URB transfer buffer\n");
> +		return -EINVAL;
> +	}

Could that BUG_ON be hit by userspace somehow?  Or is this just an
internal check for the api usage?

And sending out a 0 buffer length might be a valid thing (or at least a
crazy attempt at something), so you might want to make that dev_dbg() in
case userspace could trigger this to keep the log spam down.

thanks,

greg k-h

Re: [PATCH] usbip: Fix vhci_urb_enqueue() URB null transfer buffer error path

[Sergei Shtylyov]

Hello!

On 01/19/2019 12:29 AM, Shuah Khan wrote:

> From: Shuah Khan <shuah@kernel.org>
> 
> Fix vhci_urb_enqueue() to print error and return error instead of
> failing with WARN_ON.

   It's BUG_ON().

> Signed-off-by: Shuah Khan <shuah@kernel.org>
> ---
>  drivers/usb/usbip/vhci_hcd.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
> index 1e592ec94ba4..849ebfde87b5 100644
> --- a/drivers/usb/usbip/vhci_hcd.c
> +++ b/drivers/usb/usbip/vhci_hcd.c
> @@ -702,8 +702,10 @@ static int vhci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb, gfp_t mem_flag
>  	}
>  	vdev = &vhci_hcd->vdev[portnum-1];
>  
> -	/* patch to usb_sg_init() is in 2.5.60 */
> -	BUG_ON(!urb->transfer_buffer && urb->transfer_buffer_length);
> +	if (!urb->transfer_buffer && urb->transfer_buffer_length) {
> +		dev_err(dev, "Null URB transfer buffer\n");
> +		return -EINVAL;
> +	}
>  
>  	spin_lock_irqsave(&vhci->lock, flags);
>  

MBR, Sergei

Re: [PATCH] usbip: Fix vhci_urb_enqueue() URB null transfer buffer error path

[shuah]

On 1/19/19 9:58 AM, Sergei Shtylyov wrote:
> Hello!
> 
> On 01/19/2019 12:29 AM, Shuah Khan wrote:
> 
>> From: Shuah Khan <shuah@kernel.org>
>>
>> Fix vhci_urb_enqueue() to print error and return error instead of
>> failing with WARN_ON.
> 
>     It's BUG_ON().

Thanks. I will fix it.

-- Shuah

Re: [PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[shuah]

On 1/19/19 1:17 AM, Greg KH wrote:
> On Fri, Jan 18, 2019 at 02:29:30PM -0700, Shuah Khan wrote:
>> From: Shuah Khan <shuah@kernel.org>
>>
>> Fix vep_free_request() to return when usb_ep and usb_request are null
>> instead of calling WARN_ON.
>>
>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>> ---
>>   drivers/usb/usbip/vudc_dev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>> index 1634d8698e15..bfc8218e3fb6 100644
>> --- a/drivers/usb/usbip/vudc_dev.c
>> +++ b/drivers/usb/usbip/vudc_dev.c
>> @@ -297,7 +297,7 @@ static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
>>   {
>>   	struct vrequest *req;
>>   
>> -	if (WARN_ON(!_ep || !_req))
>> +	if (!_ep || !_req)
> 
> It's impossible for _ep to be NULL in this callback (see
> usb_ep_free_request() for where this is called from to prove that), so I
> don't think you need to check that.  It's almost impossible for _req to
> be NULL, so you might as well leave that check in.
> 

Yes. ep can never be null here in vep_free_request(). I will leave
this alone.

thanks,
-- Shuah

Re: [PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[Greg KH]

On Tue, Jan 22, 2019 at 04:05:28PM -0700, shuah wrote:
> On 1/19/19 1:17 AM, Greg KH wrote:
> > On Fri, Jan 18, 2019 at 02:29:30PM -0700, Shuah Khan wrote:
> > > From: Shuah Khan <shuah@kernel.org>
> > > 
> > > Fix vep_free_request() to return when usb_ep and usb_request are null
> > > instead of calling WARN_ON.
> > > 
> > > Signed-off-by: Shuah Khan <shuah@kernel.org>
> > > ---
> > >   drivers/usb/usbip/vudc_dev.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
> > > index 1634d8698e15..bfc8218e3fb6 100644
> > > --- a/drivers/usb/usbip/vudc_dev.c
> > > +++ b/drivers/usb/usbip/vudc_dev.c
> > > @@ -297,7 +297,7 @@ static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
> > >   {
> > >   	struct vrequest *req;
> > > -	if (WARN_ON(!_ep || !_req))
> > > +	if (!_ep || !_req)
> > 
> > It's impossible for _ep to be NULL in this callback (see
> > usb_ep_free_request() for where this is called from to prove that), so I
> > don't think you need to check that.  It's almost impossible for _req to
> > be NULL, so you might as well leave that check in.
> > 
> 
> Yes. ep can never be null here in vep_free_request(). I will leave
> this alone.

You can drop the !_ep check at the least, no need to check something
that is impossible to hit :)

thanks,

greg k-h

Re: [PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[shuah]

On 1/25/19 1:02 AM, Greg KH wrote:
> On Tue, Jan 22, 2019 at 04:05:28PM -0700, shuah wrote:
>> On 1/19/19 1:17 AM, Greg KH wrote:
>>> On Fri, Jan 18, 2019 at 02:29:30PM -0700, Shuah Khan wrote:
>>>> From: Shuah Khan <shuah@kernel.org>
>>>>
>>>> Fix vep_free_request() to return when usb_ep and usb_request are null
>>>> instead of calling WARN_ON.
>>>>
>>>> Signed-off-by: Shuah Khan <shuah@kernel.org>
>>>> ---
>>>>    drivers/usb/usbip/vudc_dev.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
>>>> index 1634d8698e15..bfc8218e3fb6 100644
>>>> --- a/drivers/usb/usbip/vudc_dev.c
>>>> +++ b/drivers/usb/usbip/vudc_dev.c
>>>> @@ -297,7 +297,7 @@ static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
>>>>    {
>>>>    	struct vrequest *req;
>>>> -	if (WARN_ON(!_ep || !_req))
>>>> +	if (!_ep || !_req)
>>>
>>> It's impossible for _ep to be NULL in this callback (see
>>> usb_ep_free_request() for where this is called from to prove that), so I
>>> don't think you need to check that.  It's almost impossible for _req to
>>> be NULL, so you might as well leave that check in.
>>>
>>
>> Yes. ep can never be null here in vep_free_request(). I will leave
>> this alone.
> 
> You can drop the !_ep check at the least, no need to check something
> that is impossible to hit :)
> 

Thanks. I will do that.

-- Shuah

[PATCH] usbip: Fix vep_free_request() null pointer checks on input args

[Shuah Khan]

Fix vep_free_request() to return when usb_ep and usb_request are null
instead of calling WARN_ON.

Signed-off-by: Shuah Khan <shuah@kernel.org>
---
 drivers/usb/usbip/vudc_dev.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 1634d8698e15..a72c17ff1c6a 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -297,7 +297,8 @@ static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
 {
 	struct vrequest *req;
 
-	if (WARN_ON(!_ep || !_req))
+	/* ep is always valid here - see usb_ep_free_request() */
+	if (!_req)
 		return;
 
 	req = to_vrequest(_req);
-- 
2.17.1