From: Yang Yingliang <[email protected]>
[ Upstream commit 649cd16c438f51d4cd777e71ca1f47f6e0c5e65d ]
If usb_set_interface() failed, iface->cur_altsetting will
not be assigned and it will be used in flexcop_usb_transfer_init()
It may lead a NULL pointer dereference.
Check usb_set_interface() return value in flexcop_usb_init()
and return failed to avoid using this NULL pointer.
Signed-off-by: Yang Yingliang <[email protected]>
Signed-off-by: Sean Young <[email protected]>
Signed-off-by: Mauro Carvalho Chehab <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
drivers/media/usb/b2c2/flexcop-usb.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
index 1a801dc286f8..d1331f828108 100644
--- a/drivers/media/usb/b2c2/flexcop-usb.c
+++ b/drivers/media/usb/b2c2/flexcop-usb.c
@@ -504,7 +504,13 @@ urb_error:
static int flexcop_usb_init(struct flexcop_usb *fc_usb)
{
/* use the alternate setting with the larges buffer */
- usb_set_interface(fc_usb->udev,0,1);
+ int ret = usb_set_interface(fc_usb->udev, 0, 1);
+
+ if (ret) {
+ err("set interface failed.");
+ return ret;
+ }
+
switch (fc_usb->udev->speed) {
case USB_SPEED_LOW:
err("cannot handle USB speed because it is too slow.");
--
2.20.1
On Sun, Dec 29, 2019 at 06:22:39PM +0100, Greg Kroah-Hartman wrote:
> From: Yang Yingliang <[email protected]>
>
> [ Upstream commit 649cd16c438f51d4cd777e71ca1f47f6e0c5e65d ]
>
> If usb_set_interface() failed, iface->cur_altsetting will
> not be assigned and it will be used in flexcop_usb_transfer_init()
> It may lead a NULL pointer dereference.
>
> Check usb_set_interface() return value in flexcop_usb_init()
> and return failed to avoid using this NULL pointer.
>
> Signed-off-by: Yang Yingliang <[email protected]>
> Signed-off-by: Sean Young <[email protected]>
> Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> Signed-off-by: Sasha Levin <[email protected]>
This commit is bogus and should be dropped from all stable queues.
Contrary to what the commit message claims, iface->cur_altsetting will
never be NULL so there's no risk for a NULL-pointer dereference here.
Even though the change itself is benign, we shouldn't spread this
confusion further.
> ---
> drivers/media/usb/b2c2/flexcop-usb.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> index 1a801dc286f8..d1331f828108 100644
> --- a/drivers/media/usb/b2c2/flexcop-usb.c
> +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> @@ -504,7 +504,13 @@ urb_error:
> static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> {
> /* use the alternate setting with the larges buffer */
> - usb_set_interface(fc_usb->udev,0,1);
> + int ret = usb_set_interface(fc_usb->udev, 0, 1);
> +
> + if (ret) {
> + err("set interface failed.");
> + return ret;
> + }
> +
> switch (fc_usb->udev->speed) {
> case USB_SPEED_LOW:
> err("cannot handle USB speed because it is too slow.");
Johan
On Fri, Jan 03, 2020 at 04:02:42PM +0100, Johan Hovold wrote:
> On Sun, Dec 29, 2019 at 06:22:39PM +0100, Greg Kroah-Hartman wrote:
> > From: Yang Yingliang <[email protected]>
> >
> > [ Upstream commit 649cd16c438f51d4cd777e71ca1f47f6e0c5e65d ]
> >
> > If usb_set_interface() failed, iface->cur_altsetting will
> > not be assigned and it will be used in flexcop_usb_transfer_init()
> > It may lead a NULL pointer dereference.
> >
> > Check usb_set_interface() return value in flexcop_usb_init()
> > and return failed to avoid using this NULL pointer.
> >
> > Signed-off-by: Yang Yingliang <[email protected]>
> > Signed-off-by: Sean Young <[email protected]>
> > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > Signed-off-by: Sasha Levin <[email protected]>
>
> This commit is bogus and should be dropped from all stable queues.
>
> Contrary to what the commit message claims, iface->cur_altsetting will
> never be NULL so there's no risk for a NULL-pointer dereference here.
Yes, you are right, I can't find any path through which cur_altsetting
will be set to NULL. The commit message is wrong. I am sorry for letting
this slip through.
Thank you for pointing this out.
> Even though the change itself is benign, we shouldn't spread this
> confusion further.
usb_set_interface() can fail for a number of reasons, and we should not
continue if it fails. So, the commit message is misleading but the
change itself is still valid.
Not sure what the right procedure is now though.
Thanks
Sean
> > ---
> > drivers/media/usb/b2c2/flexcop-usb.c | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/usb/b2c2/flexcop-usb.c b/drivers/media/usb/b2c2/flexcop-usb.c
> > index 1a801dc286f8..d1331f828108 100644
> > --- a/drivers/media/usb/b2c2/flexcop-usb.c
> > +++ b/drivers/media/usb/b2c2/flexcop-usb.c
> > @@ -504,7 +504,13 @@ urb_error:
> > static int flexcop_usb_init(struct flexcop_usb *fc_usb)
> > {
> > /* use the alternate setting with the larges buffer */
> > - usb_set_interface(fc_usb->udev,0,1);
> > + int ret = usb_set_interface(fc_usb->udev, 0, 1);
> > +
> > + if (ret) {
> > + err("set interface failed.");
> > + return ret;
> > + }
> > +
> > switch (fc_usb->udev->speed) {
> > case USB_SPEED_LOW:
> > err("cannot handle USB speed because it is too slow.");
>
> Johan
On Fri, Jan 03, 2020 at 04:12:07PM +0000, Sean Young wrote:
> On Fri, Jan 03, 2020 at 04:02:42PM +0100, Johan Hovold wrote:
> > On Sun, Dec 29, 2019 at 06:22:39PM +0100, Greg Kroah-Hartman wrote:
> > > From: Yang Yingliang <[email protected]>
> > >
> > > [ Upstream commit 649cd16c438f51d4cd777e71ca1f47f6e0c5e65d ]
> > >
> > > If usb_set_interface() failed, iface->cur_altsetting will
> > > not be assigned and it will be used in flexcop_usb_transfer_init()
> > > It may lead a NULL pointer dereference.
> > >
> > > Check usb_set_interface() return value in flexcop_usb_init()
> > > and return failed to avoid using this NULL pointer.
> > >
> > > Signed-off-by: Yang Yingliang <[email protected]>
> > > Signed-off-by: Sean Young <[email protected]>
> > > Signed-off-by: Mauro Carvalho Chehab <[email protected]>
> > > Signed-off-by: Sasha Levin <[email protected]>
> >
> > This commit is bogus and should be dropped from all stable queues.
> >
> > Contrary to what the commit message claims, iface->cur_altsetting will
> > never be NULL so there's no risk for a NULL-pointer dereference here.
>
> Yes, you are right, I can't find any path through which cur_altsetting
> will be set to NULL. The commit message is wrong. I am sorry for letting
> this slip through.
>
> Thank you for pointing this out.
>
> > Even though the change itself is benign, we shouldn't spread this
> > confusion further.
>
> usb_set_interface() can fail for a number of reasons, and we should not
> continue if it fails. So, the commit message is misleading but the
> change itself is still valid.
Sure, the change itself is fine, but I wouldn't consider it stable
material even with a correct commit message as it is not a critical fix.
And the user would still see an error message in case changing
altsetting fails.
Johan