media: fix USB descriptor issues
Subsystems (6)
cover media: fix USB descriptor issues
This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.
Johan
Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media: xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check
drivers/media/rc/iguanair.c | 2 +- drivers/media/usb/b2c2/flexcop-usb.c | 6 +++--- drivers/media/usb/dvb-usb/dib0700_core.c | 4 ++-- drivers/media/usb/gspca/ov519.c | 10 ++++++++++ drivers/media/usb/gspca/stv06xx/stv06xx.c | 19 ++++++++++++++++++- .../media/usb/gspca/stv06xx/stv06xx_pb0100.c | 4 ++++ drivers/media/usb/gspca/xirlink_cit.c | 18 +++++++++++++++++- 7 files changed, 55 insertions(+), 8 deletions(-)
Signature · 1 line
2.24.1
PATCH 1/6 media: flexcop-usb: fix endpoint sanity check
A recent commit added an endpoint sanity check to address a NULL-pointer dereference on probe. Unfortunately the check was done on the current altsetting which was later changed.
Fix this by moving the sanity check to after the altsetting is changed.
--- drivers/media/usb/b2c2/flexcop-usb.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/media/usb/b2c2/flexcop-usb.c +3 -3
@@ -511,6 +511,9 @@ static int flexcop_usb_init(struct flexcop_usb *fc_usb)return ret; } +if (fc_usb->uintf->cur_altsetting->desc.bNumEndpoints <1 ) +return -ENODEV ; +switch (fc_usb->udev->speed) {case USB_SPEED_LOW :err ("cannot handle USB speed because it is too slow." );
@@ -544,9 +547,6 @@ static int flexcop_usb_probe(struct usb_interface *intf,struct flexcop_device *fc =NULL ;int ret; -if (intf->cur_altsetting->desc.bNumEndpoints <1 ) -return -ENODEV ; -if ((fc =flexcop_device_kmalloc (sizeof (struct flexcop_usb ))) ==NULL ) {err ("out of memory\n" );return -ENOMEM ;
Signature · 1 line
2.24.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH 1/6] media: flexcop-usb: fix endpoint sanity check
Quoted 18 lines · Sasha Levin
Hi,
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag, fixing commit:
1b976fc6d684("media: b2c2-flexcop-usb: add sanity checking").The bot has tested the following trees: v5.4.7, v5.3.18, v4.19.92, v4.14.161, v4.9.207, v4.4.207.
v5.4.7: Build OK!
v5.3.18: Failed to apply! Possible dependencies:
649cd16c438f("media: flexcop-usb: fix NULL-ptr deref inflexcop_usb_transfer_init()")v4.19.92: Build OK!
v4.14.161: Build OK!
v4.9.207: Failed to apply! Possible dependencies:
649cd16c438f("media: flexcop-usb: fix NULL-ptr deref inflexcop_usb_transfer_init()")
Quoted 7 lines
v4.4.207: Failed to apply! Possible dependencies:
649cd16c438f("media: flexcop-usb: fix NULL-ptr deref inflexcop_usb_transfer_init()")NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
Just fix the trivial context change when backporting.
Johan
PATCH 2/6 media: ov519: add missing endpoint sanity checks
In reply to Johan HovoldMake sure to check that we have at least one endpoint before accessing the endpoint array to avoid dereferencing a NULL-pointer on stream start.
Note that these sanity checks are not redundant as the driver is mixing looking up altsettings by index and by number, which need not coincide.
--- drivers/media/usb/gspca/ov519.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
drivers/media/usb/gspca/ov519.c +10 -0
@@ -3477,6 +3477,11 @@ static void ov511_mode_init_regs(struct sd *sd)return ; } +if (alt->desc.bNumEndpoints <1 ) { + sd->gspca_dev.usb_err = -ENODEV ; +return ; + } + packet_size =le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize);reg_w (sd, R51x_FIFO_PSIZE, packet_size >>5 );
@@ -3603,6 +3608,11 @@ static void ov518_mode_init_regs(struct sd *sd)return ; } +if (alt->desc.bNumEndpoints <1 ) { + sd->gspca_dev.usb_err = -ENODEV ; +return ; + } + packet_size =le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize);ov518_reg_w32 (sd, R51x_FIFO_PSIZE, packet_size & ~7 ,2 );
Signature · 1 line
2.24.1
PATCH 3/6 media: stv06xx: add missing descriptor sanity checks
In reply to Johan HovoldMake sure to check that we have two alternate settings and at least one endpoint before accessing the second altsetting structure and dereferencing the endpoint arrays.
This specifically avoids dereferencing NULL-pointers or corrupting memory when a device does not have the expected descriptors.
Note that the sanity checks in stv06xx_start() and pb0100_start() are not redundant as the driver is mixing looking up altsettings by index and by number, which may not coincide.
--- drivers/media/usb/gspca/stv06xx/stv06xx.c | 19 ++++++++++++++++++- .../media/usb/gspca/stv06xx/stv06xx_pb0100.c | 4 ++++ 2 files changed, 22 insertions(+), 1 deletion(-)
drivers/media/usb/gspca/stv06xx/stv06xx.c +18 -1
@@ -282,6 +282,9 @@ static int stv06xx_start(struct gspca_dev *gspca_dev)return -EIO ; } +if (alt->desc.bNumEndpoints <1 ) +return -ENODEV ; + packet_size =le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize); err =stv06xx_write_bridge (sd,STV_ISO_SIZE_L , packet_size);if (err <0 )
@@ -306,11 +309,21 @@ static int stv06xx_start(struct gspca_dev *gspca_dev)static int stv06xx_isoc_init (struct gspca_dev *gspca_dev) { +struct usb_interface_cache *intfc;struct usb_host_interface *alt;struct sd *sd = (struct sd *) gspca_dev; + intfc = gspca_dev->dev->actconfig->intf_cache[0 ]; + +if (intfc->num_altsetting <2 ) +return -ENODEV ; + + alt = &intfc->altsetting[1 ]; + +if (alt->desc.bNumEndpoints <1 ) +return -ENODEV ; +/* Start isoc bandwidth "negotiation" at max isoc bandwidth */ - alt = &gspca_dev->dev->actconfig->intf_cache[0 ]->altsetting[1 ]; alt->endpoint[0 ].desc.wMaxPacketSize =cpu_to_le16 (sd->sensor->max_packet_size[gspca_dev->curr_mode]);
@@ -323,6 +336,10 @@ static int stv06xx_isoc_nego(struct gspca_dev *gspca_dev)struct usb_host_interface *alt;struct sd *sd = (struct sd *) gspca_dev; +/* +* Existence of altsetting and endpoint was verified in +* stv06xx_isoc_init() +*/ alt = &gspca_dev->dev->actconfig->intf_cache[0 ]->altsetting[1 ]; packet_size =le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize); min_packet_size = sd->sensor->min_packet_size[gspca_dev->curr_mode];
drivers/media/usb/gspca/stv06xx/stv06xx_pb0100.c +4 -0
@@ -185,6 +185,10 @@ static int pb0100_start(struct sd *sd) alt =usb_altnum_to_altsetting (intf, sd->gspca_dev.alt);if (!alt)return -ENODEV ; + +if (alt->desc.bNumEndpoints <1 ) +return -ENODEV ; + packet_size =le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize);/* If we don't have enough bandwidth use a lower framerate */
Signature · 1 line
2.24.1
PATCH 4/6 media: xirlink_cit: add missing descriptor sanity checks
In reply to Johan HovoldMake sure to check that we have two alternate settings and at least one endpoint before accessing the second altsetting structure and dereferencing the endpoint arrays.
This specifically avoids dereferencing NULL-pointers or corrupting memory when a device does not have the expected descriptors.
Note that the sanity check in cit_get_packet_size() is not redundant as the driver is mixing looking up altsettings by index and by number, which may not coincide.
--- drivers/media/usb/gspca/xirlink_cit.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
drivers/media/usb/gspca/xirlink_cit.c +17 -1
@@ -1442,6 +1442,9 @@ static int cit_get_packet_size(struct gspca_dev *gspca_dev)return -EIO ; } +if (alt->desc.bNumEndpoints <1 ) +return -ENODEV ; +return le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize); }
@@ -2626,6 +2629,7 @@ static int sd_start(struct gspca_dev *gspca_dev)static int sd_isoc_init (struct gspca_dev *gspca_dev) { +struct usb_interface_cache *intfc;struct usb_host_interface *alt;int max_packet_size;
@@ -2641,8 +2645,17 @@ static int sd_isoc_init(struct gspca_dev *gspca_dev)break ; } + intfc = gspca_dev->dev->actconfig->intf_cache[0 ]; + +if (intfc->num_altsetting <2 ) +return -ENODEV ; + + alt = &intfc->altsetting[1 ]; + +if (alt->desc.bNumEndpoints <1 ) +return -ENODEV ; +/* Start isoc bandwidth "negotiation" at max isoc bandwidth */ - alt = &gspca_dev->dev->actconfig->intf_cache[0 ]->altsetting[1 ]; alt->endpoint[0 ].desc.wMaxPacketSize =cpu_to_le16 (max_packet_size);return 0 ;
@@ -2665,6 +2678,9 @@ static int sd_isoc_nego(struct gspca_dev *gspca_dev)break ; } +/* +* Existence of altsetting and endpoint was verified in sd_isoc_init() +*/ alt = &gspca_dev->dev->actconfig->intf_cache[0 ]->altsetting[1 ]; packet_size =le16_to_cpu (alt->endpoint[0 ].desc.wMaxPacketSize);if (packet_size <= min_packet_size)
Signature · 1 line
2.24.1
PATCH 5/6 media: dib0700: fix rc endpoint lookup
In reply to Johan HovoldMake sure to use the current alternate setting when verifying the interface descriptors to avoid submitting an URB to an invalid endpoint.
Failing to do so could cause the driver to misbehave or trigger a WARN() in usb_submit_urb() that kernels with panic_on_warn set would choke on.
--- drivers/media/usb/dvb-usb/dib0700_core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/usb/dvb-usb/dib0700_core.c +2 -2
@@ -818,7 +818,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d, struct usb_interface *intf)/* Starting in firmware 1.20, the RC info is provided on a bulk pipe */ -if (intf->altsetting[0 ].desc.bNumEndpoints < rc_ep +1 ) +if (intf->cur_altsetting->desc.bNumEndpoints < rc_ep +1 )return -ENODEV ; purb =usb_alloc_urb (0 ,GFP_KERNEL );
@@ -838,7 +838,7 @@ int dib0700_rc_setup(struct dvb_usb_device *d, struct usb_interface *intf)* Some devices like the Hauppauge NovaTD model 52009 use an interrupt * endpoint, while others use a bulk one. */ - e = &intf->altsetting[0 ].endpoint[rc_ep].desc; + e = &intf->cur_altsetting->endpoint[rc_ep].desc;if (usb_endpoint_dir_in (e)) {if (usb_endpoint_xfer_bulk (e)) { pipe =usb_rcvbulkpipe (d->udev, rc_ep);
Signature · 1 line
2.24.1
PATCH 6/6 media: iguanair: fix endpoint sanity check
In reply to Johan HovoldMake sure to use the current alternate setting, which need not be the first one by index, when verifying the endpoint descriptors and initialising the URBs.
Failing to do so could cause the driver to misbehave or trigger a WARN() in usb_submit_urb() that kernels with panic_on_warn set would choke on.
--- drivers/media/rc/iguanair.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/rc/iguanair.c +1 -1
@@ -413,7 +413,7 @@ static int iguanair_probe(struct usb_interface *intf,int ret, pipein, pipeout;struct usb_host_interface *idesc; - idesc = intf->altsetting; + idesc = intf->cur_altsetting;if (idesc->desc.bNumEndpoints <2 )return -ENODEV ;
Signature · 1 line
2.24.1
Quoted 11 lines · Johan Hovold
This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.
Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media:xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check
Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.
Johan
Quoted 13 lines · Johan Hovold
Quoted 11 lines · Johan Hovold
This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.
Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media:xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check
Quoted 2 lines
Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.
Another month, another reminder. Three of the above patches still hasn't been applied.
Johan
Quoted 14 lines · Johan Hovold
Quoted 13 lines · Johan Hovold
Quoted 11 lines · Johan Hovold
This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.
Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media:xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check
Quoted 5 lines
Quoted 2 lines
Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.
Another month, another reminder. Three of the above patches still hasn't been applied.
I've delegated these to me and will make a PR today/tomorrow.
Regards,
Hans
Quoted 15 lines · Hans Verkuil
Quoted 14 lines · Johan Hovold
Quoted 13 lines · Johan Hovold
Quoted 11 lines · Johan Hovold
This series fixes a number of issues due to missing or incomplete sanity checks that could lead to NULL-pointer dereferences, memory corruption or driver misbehaviour when a device has unexpected descriptors.
Johan Hovold (6):
media: flexcop-usb: fix endpoint sanity check
media: ov519: add missing endpoint sanity checks
media: stv06xx: add missing descriptor sanity checks
media:xirlink_cit: add missing descriptor sanity checks
media: dib0700: fix rc endpoint lookup
media: iguanair: fix endpoint sanity check
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Just sending a reminder about these as it seems only the last one has made into mainline (and stable) yet.
Another month, another reminder. Three of the above patches still hasn't been applied.
I've delegated these to me and will make a PR today/tomorrow.
Thanks, Hans!
Johan