Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 7 May 2019 17:07:25 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Alan Stern <stern@rowland.harvard.edu>
Cc:     Johan Hovold <johan@kernel.org>,
        syzbot <syzbot+53f029db71c19a47325a@syzkaller.appspotmail.com>,
        andreyknvl@google.com, linux-kernel@vger.kernel.org,
        linux-media@vger.kernel.org, linux-usb@vger.kernel.org,
        mchehab@kernel.org, syzkaller-bugs@googlegroups.com,
        wen.yang99@zte.com.cn
Subject: Re: general protection fault in smsusb_init_device
Message-ID: <20190507150725.GB29016@localhost>
References: <20190507083430.GD4333@localhost>
 <Pine.LNX.4.44L0.1905071035450.1632-100000@iolanthe.rowland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Pine.LNX.4.44L0.1905071035450.1632-100000@iolanthe.rowland.org>
User-Agent: Mutt/1.11.4 (2019-03-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, May 07, 2019 at 10:42:58AM -0400, Alan Stern wrote:
> On Tue, 7 May 2019, Johan Hovold wrote:

> > > @@ -411,6 +412,23 @@ static int smsusb_init_device(struct usb
> > >  	dev->udev = interface_to_usbdev(intf);
> > >  	dev->state = SMSUSB_DISCONNECTED;
> > >  
> > > +	for (i = 0; i < intf->cur_altsetting->desc.bNumEndpoints; i++) {
> > > +		struct usb_endpoint_descriptor *desc =
> > > +				&intf->cur_altsetting->endpoint[i].desc;
> > > +
> > > +		if (desc->bEndpointAddress & USB_DIR_IN) {
> > > +			dev->in_ep = desc->bEndpointAddress;
> > > +			in_maxp = usb_endpoint_maxp(desc);
> > > +		} else {
> > > +			dev->out_ep = desc->bEndpointAddress;
> > > +		}
> > > +	}
> > > +
> > > +	pr_debug("in_ep = %02x, out_ep = %02x\n",
> > > +		dev->in_ep, dev->out_ep);
> > > +	if (!dev->in_ep || !dev->out_ep)	/* Missing endpoints? */
> > > +		return -EINVAL;
> > 
> > Looks like you're now leaking dev here, and so is the current code in
> > the later error paths.
> > 
> > Since this return value will be returned from probe, you may want to use
> > -ENXIO or -ENODEV instead of -EINVAL.
> > 
> > Looks good otherwise.
> 
> Thanks for the review.  You're right about the memory leak (although 
> you're wrong about the later error paths: smsusb_term_device() 
> deallocates dev).

Indeed, I missed the free in smsusb_term_device(). Sorry about that.

Johan