The driver submits bulk urb without checking the endpoint type is
actually bulk.
[ 3.108411] ------------[ cut here ]------------
[ 3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1
[ 3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0
[ 3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0
[ 3.115318] Call Trace:
[ 3.115452] <TASK>
[ 3.115570] try_queue_bulk_in+0x43c/0x6e0 [xillyusb]
[ 3.115838] xillyusb_probe+0x488/0x1230 [xillyusb]
Add a check to fix it.
Fixes: a53d1202aef1 ("char: xillybus: Add driver for XillyUSB (Xillybus variant for USB)")
Signed-off-by: Zheyu Ma <[email protected]>
---
drivers/char/xillybus/xillyusb.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index 39bcbfd908b4..2ec2e087b2e7 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -730,6 +730,11 @@ static void try_queue_bulk_in(struct xillyusb_endpoint *ep)
usb_anchor_urb(urb, &ep->anchor);
+ if (usb_urb_ep_type_check(urb)) {
+ report_io_error(xdev, -EINVAL);
+ goto unanchor;
+ }
+
rc = usb_submit_urb(urb, GFP_KERNEL);
if (rc) {
@@ -834,6 +839,11 @@ static void try_queue_bulk_out(struct xillyusb_endpoint *ep)
usb_anchor_urb(urb, &ep->anchor);
+ if (usb_urb_ep_type_check(urb)) {
+ report_io_error(xdev, -EINVAL);
+ goto unanchor;
+ }
+
rc = usb_submit_urb(urb, GFP_KERNEL);
if (rc) {
--
2.25.1
On Sat, May 14, 2022 at 03:14:35PM +0800, Zheyu Ma wrote:
> The driver submits bulk urb without checking the endpoint type is
> actually bulk.
>
> [ 3.108411] ------------[ cut here ]------------
> [ 3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> [ 3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0
> [ 3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0
> [ 3.115318] Call Trace:
> [ 3.115452] <TASK>
> [ 3.115570] try_queue_bulk_in+0x43c/0x6e0 [xillyusb]
> [ 3.115838] xillyusb_probe+0x488/0x1230 [xillyusb]
>
> Add a check to fix it.
>
> Fixes: a53d1202aef1 ("char: xillybus: Add driver for XillyUSB (Xillybus variant for USB)")
> Signed-off-by: Zheyu Ma <[email protected]>
> ---
> drivers/char/xillybus/xillyusb.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
> index 39bcbfd908b4..2ec2e087b2e7 100644
> --- a/drivers/char/xillybus/xillyusb.c
> +++ b/drivers/char/xillybus/xillyusb.c
> @@ -730,6 +730,11 @@ static void try_queue_bulk_in(struct xillyusb_endpoint *ep)
>
> usb_anchor_urb(urb, &ep->anchor);
>
> + if (usb_urb_ep_type_check(urb)) {
> + report_io_error(xdev, -EINVAL);
> + goto unanchor;
> + }
> +
> rc = usb_submit_urb(urb, GFP_KERNEL);
>
> if (rc) {
> @@ -834,6 +839,11 @@ static void try_queue_bulk_out(struct xillyusb_endpoint *ep)
>
> usb_anchor_urb(urb, &ep->anchor);
>
> + if (usb_urb_ep_type_check(urb)) {
> + report_io_error(xdev, -EINVAL);
> + goto unanchor;
> + }
> +
> rc = usb_submit_urb(urb, GFP_KERNEL);
>
> if (rc) {
> --
> 2.25.1
>
This should all be checked much earlier, in your probe function, to make
sure you have the endpoints correct. Just refuse to bind to a device
that is not correct. Don't check late here when you are submitting the
urb.
thanks,
greg k-h
The driver submits bulk urb without checking the endpoint type is
actually bulk.
[ 3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1
[ 3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0
[ 3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0
[ 3.115318] Call Trace:
[ 3.115452] <TASK>
[ 3.115570] try_queue_bulk_in+0x43c/0x6e0 [xillyusb]
[ 3.115838] xillyusb_probe+0x488/0x1230 [xillyusb]
Add a check in endpoint_alloc() to fix the bug.
Signed-off-by: Zheyu Ma <[email protected]>
---
Changes in v2:
- Check the endpoint type at probe time
---
drivers/char/xillybus/xillyusb.c | 27 ++++++++++++++++++++++++++-
1 file changed, 26 insertions(+), 1 deletion(-)
diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
index dc3551796e5e..4467f13993ef 100644
--- a/drivers/char/xillybus/xillyusb.c
+++ b/drivers/char/xillybus/xillyusb.c
@@ -167,6 +167,7 @@ struct xillyusb_dev {
struct device *dev; /* For dev_err() and such */
struct kref kref;
struct workqueue_struct *workq;
+ struct usb_interface *intf;
int error;
spinlock_t error_lock; /* protect @error */
@@ -475,6 +476,25 @@ static void endpoint_dealloc(struct xillyusb_endpoint *ep)
kfree(ep);
}
+static int xillyusb_check_endpoint(struct xillyusb_dev *xdev, u8 ep_num)
+{
+ struct usb_host_interface *if_desc = xdev->intf->altsetting;
+ int i;
+
+ for (i = 0; i < if_desc->desc.bNumEndpoints; i++) {
+ struct usb_endpoint_descriptor *ep = &if_desc->endpoint[i].desc;
+
+ if (ep->bEndpointAddress != ep_num)
+ continue;
+
+ if ((usb_pipein(ep_num) && usb_endpoint_is_bulk_in(ep)) ||
+ (usb_pipeout(ep_num) && usb_endpoint_is_bulk_out(ep)))
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
static struct xillyusb_endpoint
*endpoint_alloc(struct xillyusb_dev *xdev,
u8 ep_num,
@@ -482,10 +502,14 @@ static struct xillyusb_endpoint
unsigned int order,
int bufnum)
{
- int i;
+ int i, ret;
struct xillyusb_endpoint *ep;
+ ret = xillyusb_check_endpoint(xdev, ep_num);
+ if (ret)
+ return NULL;
+
ep = kzalloc(sizeof(*ep), GFP_KERNEL);
if (!ep)
@@ -2125,6 +2149,7 @@ static int xillyusb_probe(struct usb_interface *interface,
mutex_init(&xdev->process_in_mutex);
mutex_init(&xdev->msg_mutex);
+ xdev->intf = interface;
xdev->udev = usb_get_dev(interface_to_usbdev(interface));
xdev->dev = &interface->dev;
xdev->error = 0;
--
2.25.1
On Fri, May 20, 2022 at 1:41 PM Greg KH <[email protected]> wrote:
>
> On Fri, May 20, 2022 at 11:32:51AM +0800, Zheyu Ma wrote:
> > On Sat, May 14, 2022 at 9:32 PM Greg KH <[email protected]> wrote:
> > >
> > > On Sat, May 14, 2022 at 07:48:19PM +0800, Zheyu Ma wrote:
> > > > The driver submits bulk urb without checking the endpoint type is
> > > > actually bulk.
> > > >
> > > > [ 3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> > > > [ 3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0
> > > > [ 3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0
> > > > [ 3.115318] Call Trace:
> > > > [ 3.115452] <TASK>
> > > > [ 3.115570] try_queue_bulk_in+0x43c/0x6e0 [xillyusb]
> > > > [ 3.115838] xillyusb_probe+0x488/0x1230 [xillyusb]
> > > >
> > > > Add a check in endpoint_alloc() to fix the bug.
> > > >
> > > > Signed-off-by: Zheyu Ma <[email protected]>
> > > > ---
> > > > Changes in v2:
> > > > - Check the endpoint type at probe time
> > > > ---
> > > > drivers/char/xillybus/xillyusb.c | 27 ++++++++++++++++++++++++++-
> > > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
> > > > index dc3551796e5e..4467f13993ef 100644
> > > > --- a/drivers/char/xillybus/xillyusb.c
> > > > +++ b/drivers/char/xillybus/xillyusb.c
> > > > @@ -167,6 +167,7 @@ struct xillyusb_dev {
> > > > struct device *dev; /* For dev_err() and such */
> > > > struct kref kref;
> > > > struct workqueue_struct *workq;
> > > > + struct usb_interface *intf;
> > > >
> > > > int error;
> > > > spinlock_t error_lock; /* protect @error */
> > > > @@ -475,6 +476,25 @@ static void endpoint_dealloc(struct xillyusb_endpoint *ep)
> > > > kfree(ep);
> > > > }
> > > >
> > > > +static int xillyusb_check_endpoint(struct xillyusb_dev *xdev, u8 ep_num)
> > > > +{
> > > > + struct usb_host_interface *if_desc = xdev->intf->altsetting;
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < if_desc->desc.bNumEndpoints; i++) {
> > > > + struct usb_endpoint_descriptor *ep = &if_desc->endpoint[i].desc;
> > > > +
> > > > + if (ep->bEndpointAddress != ep_num)
> > > > + continue;
> > > > +
> > > > + if ((usb_pipein(ep_num) && usb_endpoint_is_bulk_in(ep)) ||
> > > > + (usb_pipeout(ep_num) && usb_endpoint_is_bulk_out(ep)))
> > > > + return 0;
> > > > + }
> > >
> > > Why not use the built-in usb core functions that do this for you instead
> > > of hand-parsing this? Look at usb_find_common_endpoints() and related
> > > functions, that should make this much easier.
> >
> > Thanks for your reminder. But in this driver, we have to check not
> > only the type and direction of the endpoint, but also the address of
> > it. And the endpoint's address is sometimes dynamic. For example, in
> > the function xillyusb_open():
> >
> > out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
> > bulk_out_work, BUF_SIZE_ORDER, BUFNUM);
> >
> > However, usb_find_common_endpoints() can only find the first endpoint
> > that satisfies the condition, not on a specific address. I cannot find
> > a more suitable built-in core function, please correct me if I'm
> > wrong.
>
> I do not understand the problem here, it looks like your code above that
> I responded to doesn't care about specific addresses at all. It is just
> walking all of them and making sure that it is a bulk in/out endpoint.
Please correct me if I'm wrong. I think the driver needs to check if
the urb has the correct type before submitting the urb, and this check
should be done early.
This driver uses endpoint_alloc() to allocate an endpoint and then
uses this endpoint for transfers. And one of the arguments of
endpoint_alloc() is 'ep_num'. So I want to iterate over the endpoints
and exclude the unwanted address as follows:
if (ep->bEndpointAddress != ep_num)
continue;
> And why do you care about the address of the endpoint? If you know
> that, then there's no need to walk them all and you can check that even
> easier.
I care about the address of the endpoint because it is an argument of
endpoint_alloc(). I don't know a better solution to check the endpoint
even if I know the address.
Thanks,
Zheyu Ma
On Sun, May 22, 2022 at 01:06:59PM +0800, Zheyu Ma wrote:
> On Fri, May 20, 2022 at 1:41 PM Greg KH <[email protected]> wrote:
> >
> > On Fri, May 20, 2022 at 11:32:51AM +0800, Zheyu Ma wrote:
> > > On Sat, May 14, 2022 at 9:32 PM Greg KH <[email protected]> wrote:
> > > >
> > > > On Sat, May 14, 2022 at 07:48:19PM +0800, Zheyu Ma wrote:
> > > > > The driver submits bulk urb without checking the endpoint type is
> > > > > actually bulk.
> > > > >
> > > > > [ 3.108690] usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> > > > > [ 3.108983] WARNING: CPU: 0 PID: 211 at drivers/usb/core/urb.c:503 usb_submit_urb+0xcd9/0x18b0
> > > > > [ 3.110976] RIP: 0010:usb_submit_urb+0xcd9/0x18b0
> > > > > [ 3.115318] Call Trace:
> > > > > [ 3.115452] <TASK>
> > > > > [ 3.115570] try_queue_bulk_in+0x43c/0x6e0 [xillyusb]
> > > > > [ 3.115838] xillyusb_probe+0x488/0x1230 [xillyusb]
> > > > >
> > > > > Add a check in endpoint_alloc() to fix the bug.
> > > > >
> > > > > Signed-off-by: Zheyu Ma <[email protected]>
> > > > > ---
> > > > > Changes in v2:
> > > > > - Check the endpoint type at probe time
> > > > > ---
> > > > > drivers/char/xillybus/xillyusb.c | 27 ++++++++++++++++++++++++++-
> > > > > 1 file changed, 26 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/char/xillybus/xillyusb.c b/drivers/char/xillybus/xillyusb.c
> > > > > index dc3551796e5e..4467f13993ef 100644
> > > > > --- a/drivers/char/xillybus/xillyusb.c
> > > > > +++ b/drivers/char/xillybus/xillyusb.c
> > > > > @@ -167,6 +167,7 @@ struct xillyusb_dev {
> > > > > struct device *dev; /* For dev_err() and such */
> > > > > struct kref kref;
> > > > > struct workqueue_struct *workq;
> > > > > + struct usb_interface *intf;
> > > > >
> > > > > int error;
> > > > > spinlock_t error_lock; /* protect @error */
> > > > > @@ -475,6 +476,25 @@ static void endpoint_dealloc(struct xillyusb_endpoint *ep)
> > > > > kfree(ep);
> > > > > }
> > > > >
> > > > > +static int xillyusb_check_endpoint(struct xillyusb_dev *xdev, u8 ep_num)
> > > > > +{
> > > > > + struct usb_host_interface *if_desc = xdev->intf->altsetting;
> > > > > + int i;
> > > > > +
> > > > > + for (i = 0; i < if_desc->desc.bNumEndpoints; i++) {
> > > > > + struct usb_endpoint_descriptor *ep = &if_desc->endpoint[i].desc;
> > > > > +
> > > > > + if (ep->bEndpointAddress != ep_num)
> > > > > + continue;
> > > > > +
> > > > > + if ((usb_pipein(ep_num) && usb_endpoint_is_bulk_in(ep)) ||
> > > > > + (usb_pipeout(ep_num) && usb_endpoint_is_bulk_out(ep)))
> > > > > + return 0;
> > > > > + }
> > > >
> > > > Why not use the built-in usb core functions that do this for you instead
> > > > of hand-parsing this? Look at usb_find_common_endpoints() and related
> > > > functions, that should make this much easier.
> > >
> > > Thanks for your reminder. But in this driver, we have to check not
> > > only the type and direction of the endpoint, but also the address of
> > > it. And the endpoint's address is sometimes dynamic. For example, in
> > > the function xillyusb_open():
> > >
> > > out_ep = endpoint_alloc(xdev, (chan->chan_idx + 2) | USB_DIR_OUT,
> > > bulk_out_work, BUF_SIZE_ORDER, BUFNUM);
> > >
> > > However, usb_find_common_endpoints() can only find the first endpoint
> > > that satisfies the condition, not on a specific address. I cannot find
> > > a more suitable built-in core function, please correct me if I'm
> > > wrong.
> >
> > I do not understand the problem here, it looks like your code above that
> > I responded to doesn't care about specific addresses at all. It is just
> > walking all of them and making sure that it is a bulk in/out endpoint.
>
> Please correct me if I'm wrong. I think the driver needs to check if
> the urb has the correct type before submitting the urb, and this check
> should be done early.
Yes, very very early, like probe() callback time early.
Not way down here in "do we want to allow open() to work or not" like
you are currently doing.
If the device does not have the EXACT USB endpoints that you are
expecting (size, address, direction, type, etc.) at probe time reject
the device.
That is what the helper functions I pointed you at are for.
This driver is trying to detect this type of problem _way_ too late.
thanks,
greg k-h
On 23/05/22 19:06, Greg KH wrote:
> If the device does not have the EXACT USB endpoints that you are
> expecting (size, address, direction, type, etc.) at probe time reject
> the device.
>
This is probably a good time to add some information about XillyUSB.
All XillyUSB devices have EP 1 IN and EP 1 OUT as bulk endpoints.
On top of that, they *may* have up to 14 additional bulk OUT endpoints,
having the addresses EP 2 OUT to EP 15 OUT. The population of endpoint
addresses is not necessarily continuous. Any set of OUT endpoint
addresses is allowed. The driver doesn't know which of these endpoints
are available initially.
Rather, it works like this: The driver uses the EP 1 IN and OUT
endpoints to query the device for a data structure, which contains
information on the device's communication channels. The driver sets up
device files accordingly, and it also gets informed on which bulk OUT
endpoints are present.
For what it's worth, I think it's fairly safe to assume that if a device
returns a legal data structure (which passes a CRC test), it's a
XillyUSB device. Either way, it's impossible to verify that all of the
device's bulk OUT endpoints are correct before obtaining the data
structure from the device. The fact that each device has a different set
of communication channels, and that the driver learns about them in
run-time is the whole trick with Xillybus and XillyUSB.
And just in case you wonder why there's only one bulk IN endpoint: All
inbound communication, control as well as data, is multiplexed into this
single endpoint. That's in order to allow the device better control on
which communication channel is serviced at any time, with a few
microseconds' granularity. The same trick is unfortunately infeasible in
the other direction.
I don't have any particular view on how the device should be validated,
but I thought this information would be helpful.
Thanks,
Eli
On Mon, May 23, 2022 at 08:05:47PM +0300, Eli Billauer wrote:
> On 23/05/22 19:06, Greg KH wrote:
> > If the device does not have the EXACT USB endpoints that you are
> > expecting (size, address, direction, type, etc.) at probe time reject
> > the device.
> This is probably a good time to add some information about XillyUSB.
>
> All XillyUSB devices have EP 1 IN and EP 1 OUT as bulk endpoints.
>
> On top of that, they *may* have up to 14 additional bulk OUT endpoints,
> having the addresses EP 2 OUT to EP 15 OUT. The population of endpoint
> addresses is not necessarily continuous. Any set of OUT endpoint addresses
> is allowed. The driver doesn't know which of these endpoints are available
> initially.
>
> Rather, it works like this: The driver uses the EP 1 IN and OUT endpoints to
> query the device for a data structure, which contains information on the
> device's communication channels. The driver sets up device files
> accordingly, and it also gets informed on which bulk OUT endpoints are
> present.
>
> For what it's worth, I think it's fairly safe to assume that if a device
> returns a legal data structure (which passes a CRC test), it's a XillyUSB
> device.
Why? You still need to verify that the requested endpoints match up
with what the device really has.
Again, we are talking about "fake" devices that are being used to find
problems in the kernel drivers, this is not a "real" device, yet can
look a lot like one. Look at the fuzzing tools that are running for
examples of this.
> Either way, it's impossible to verify that all of the device's bulk
> OUT endpoints are correct before obtaining the data structure from the
> device. The fact that each device has a different set of communication
> channels, and that the driver learns about them in run-time is the whole
> trick with Xillybus and XillyUSB.
That's fine, but that can still be done a probe() time, not open() time,
like the current patch was attempting. That's much too late.
thanks,
greg k-h
On 23/05/22 20:15, Greg KH wrote:
>> Rather, it works like this: The driver uses the EP 1 IN and OUT endpoints to
>> > query the device for a data structure, which contains information on the
>> > device's communication channels. The driver sets up device files
>> > accordingly, and it also gets informed on which bulk OUT endpoints are
>> > present.
>> >
>> > For what it's worth, I think it's fairly safe to assume that if a device
>> > returns a legal data structure (which passes a CRC test), it's a XillyUSB
>> > device.
>>
> Why? You still need to verify that the requested endpoints match up
> with what the device really has.
>
OK. So to summarize:
EP 1 IN and EP 1 OUT are always present in XillyUSB devices.
On top of these, there might be additional bulk OUT endpoints. The
driver resolves which ones in setup_channels(), by scanning a data blob
it has received from the device. This takes place in the for-loop inside
this function. When (out_desc & 0x80) is true for a given @i in the
loop, the device has a bulk OUT endpoint with address i+2 (e.g. if this
condition is met for i==2, the device has a bulk OUT EP 4).
So it seems like setup_channels() would be the right place to make this
check, since it's called during the device's probe.
I guess it would likewise make sense to check for EP 1 IN and OUT in
xillyusb_setup_base_eps().
Thanks,
Eli
On Tue, May 24, 2022 at 8:24 PM Eli Billauer <[email protected]> wrote:
>
> On 23/05/22 20:15, Greg KH wrote:
> >> Rather, it works like this: The driver uses the EP 1 IN and OUT endpoints to
> >> > query the device for a data structure, which contains information on the
> >> > device's communication channels. The driver sets up device files
> >> > accordingly, and it also gets informed on which bulk OUT endpoints are
> >> > present.
> >> >
> >> > For what it's worth, I think it's fairly safe to assume that if a device
> >> > returns a legal data structure (which passes a CRC test), it's a XillyUSB
> >> > device.
> >>
> > Why? You still need to verify that the requested endpoints match up
> > with what the device really has.
> >
> OK. So to summarize:
>
> EP 1 IN and EP 1 OUT are always present in XillyUSB devices.
>
> On top of these, there might be additional bulk OUT endpoints. The
> driver resolves which ones in setup_channels(), by scanning a data blob
> it has received from the device. This takes place in the for-loop inside
> this function. When (out_desc & 0x80) is true for a given @i in the
> loop, the device has a bulk OUT endpoint with address i+2 (e.g. if this
> condition is met for i==2, the device has a bulk OUT EP 4).
>
> So it seems like setup_channels() would be the right place to make this
> check, since it's called during the device's probe.
>
> I guess it would likewise make sense to check for EP 1 IN and OUT in
> xillyusb_setup_base_eps().
>
> Thanks,
> Eli
Thanks for the detailed explanation, I will try to send a new version
of the patch to fix the bug.
Thanks,
Zheyu Ma