linux-kernel - Re: [PATCH] staging: wlan-ng: properly check endpoint types

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] [day] [month] [year] [list]

Message-ID: <20200719181223.GA210062@thinkpad>
Date:   Sun, 19 Jul 2020 11:12:23 -0700
From:   Rustam Kovhaev <rkovhaev@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     devel@...verdev.osuosl.org,
        syzbot+c2a1fa67c02faa0de723@...kaller.appspotmail.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: wlan-ng: properly check endpoint types

On Sun, Jul 19, 2020 at 11:23:38AM +0200, Greg KH wrote:
> On Sat, Jul 18, 2020 at 08:58:36AM -0700, Rustam Kovhaev wrote:
> > As syzkaller detected, wlan-ng driver submits bulk urb without checking
> > that the endpoint type is actually bulk, add usb_urb_ep_type_check()
> > 
> > Reported-and-tested-by: syzbot+c2a1fa67c02faa0de723@...kaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=c2a1fa67c02faa0de723
> > Signed-off-by: Rustam Kovhaev <rkovhaev@...il.com>
> > ---
> >  drivers/staging/wlan-ng/hfa384x_usb.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/staging/wlan-ng/hfa384x_usb.c b/drivers/staging/wlan-ng/hfa384x_usb.c
> > index fa1bf8b069fd..7cde60ea68a2 100644
> > --- a/drivers/staging/wlan-ng/hfa384x_usb.c
> > +++ b/drivers/staging/wlan-ng/hfa384x_usb.c
> > @@ -339,6 +339,12 @@ static int submit_rx_urb(struct hfa384x *hw, gfp_t memflags)
> >  
> >  	hw->rx_urb_skb = skb;
> >  
> > +	result = usb_urb_ep_type_check(&hw->rx_urb);
> > +	if (result) {
> > +	       netdev_warn(hw->wlandev->netdev, "invalid rx endpoint");
> > +	       goto cleanup;
> > +	}
> 
> In looking at this again, can you just make these checks in the probe
> function, and abort binding the driver to the device at that point in
> time?  This feels really late in the init sequence.
> 
> The real problem here is in the hfa384x_create() function, where it
> blindly takes the 1 and 2 endpoints and assumes that those are the
> "correct type".  How about checking the types there, and if they are
> incorrect, returning an error from that function and have the caller
> return the error as well.
> 
> That should keep anything else in the driver from being initialized and
> set up, and stop bad devices from being bound to the driver at a much
> earlier point in time.
> 
> Note, just checking for the valid type/direction of those endpoints
> should be sufficient.
> 
tyvm for the feedback! makes perfect sense, i'll send a new patch