linux-kernel - Re: [PATCH 5.4 106/434] media: flexcop-usb: fix NULL-ptr deref in flexcop_usb_transfer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 3 Jan 2020 16:12:07 +0000
From:   Sean Young <sean@...s.org>
To:     Johan Hovold <johan@...nel.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, stable@...r.kernel.org,
        Yang Yingliang <yangyingliang@...wei.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        Sasha Levin <sashal@...nel.org>
Subject: Re: [PATCH 5.4 106/434] media: flexcop-usb: fix NULL-ptr deref in
 flexcop_usb_transfer_init()

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 <yangyingliang@...wei.com>
> > 
> > [ 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 <yangyingliang@...wei.com>
> > Signed-off-by: Sean Young <sean@...s.org>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@...nel.org>
> > Signed-off-by: Sasha Levin <sashal@...nel.org>
> 
> 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