From: Greg KH
> Sent: 07 February 2017 10:52
> To: Petko Manolov
> Cc: Ben Hutchings; David Laight; netdev@vger.kernel.org;
> linux-...@vger.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
