Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 19 Jul 2020 11:12:23 -0700
From:   Rustam Kovhaev <rkovhaev@gmail.com>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     devel@driverdev.osuosl.org,
        syzbot+c2a1fa67c02faa0de723@syzkaller.appspotmail.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: wlan-ng: properly check endpoint types
Message-ID: <20200719181223.GA210062@thinkpad>
References: <20200718155836.86384-1-rkovhaev@gmail.com>
 <20200719092338.GC171181@kroah.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200719092338.GC171181@kroah.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@syzkaller.appspotmail.com
> > Link: https://syzkaller.appspot.com/bug?extid=c2a1fa67c02faa0de723
> > Signed-off-by: Rustam Kovhaev <rkovhaev@gmail.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