netdev - RE: [PATCH net 2/4] rtl8150: Use heap buffers for all register access

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]

Message-ID: <063D6719AE5E284EB5DD2968C1650D6DB027DB75@AcuExch.aculab.com>
Date:   Tue, 7 Feb 2017 11:56:51 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Greg KH' <greg@...ah.com>, Petko Manolov <petkan@...leusys.com>
CC:     Ben Hutchings <ben@...adent.org.uk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: RE: [PATCH net 2/4] rtl8150: Use heap buffers for all register
 access

From: Greg KH
> Sent: 07 February 2017 10:52
> To: Petko Manolov
> Cc: Ben Hutchings; David Laight; netdev@...r.kernel.org; linux-usb@...r.kernel.org
> Subject: Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access
> 
> On Tue, Feb 07, 2017 at 12:34:52PM +0200, Petko Manolov wrote:
> > On 17-02-06 16:25:20, Ben Hutchings wrote:
> > > On Mon, Feb 06, 2017 at 04:09:18PM +0000, David Laight wrote:
> > > > From: Ben Hutchings
> > > [...]
> > > > > +	ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
> > > > > +			      RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
> > > > > +			      indx, 0, buf, size, 500);
> > > > > +	if (ret > 0 && ret <= size)
> > > > > +		memcpy(data, buf, ret);
> > > >
> > > > If ret > size something is horridly wrong.
> > > > Silently not updating the callers buffer at all cannot be right.
> > >
> > > Yes, it seems strange to check this.  I originally wrote this as ret >
> > > 0, but then I checked the usbnet core and found __usbnet_read_cmd()
> > > has the second comparison as well.
> > >
> > > > > +	kfree(buf);
> > > > > +	return ret;
> >
> > Since we return what usb_control_msg() told us to return i assume the error code
> > will be available to anybody who cares.
> >
> > > > I can't help feeling that it would be better to add a wrapper to
> > > > usb_control_msg() that does the kmalloc() and memcpy()s and
> > > > drop that into all the call sites.
> > >
> > > It might be.  Right now I'm trying to patch up a bunch of regressions rather
> > > than argue over an API change.
> >
> > Right, first thing first.
> >
> > I am in favor of changing the API, but this should not happen in the stable
> > releases.  I hope Greg will make up his mind and let us know.
> 
> make up my mind about what?  These are bugs, they should be fixed, I'm
> not taking a total api change at this point in time, sorry.

Adding a usb_control_msg_with_malloc() wrapper is a smaller change than those
proposed. The smaller churn probably makes back porting other changes easier.

Given the nature of this problem, and how common it seems to be,
it is almost worth renaming usb_control_msg() itself so that all the
callers are properly audited.

	David