Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()

From: Vladis Dronov <vdronov@redhat.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: Andrey Konovalov <andreyknvl@google.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Oliver Neukum <oneukum@suse.com>,
	Hans de Goede <hdegoede@redhat.com>,
	syzkaller <syzkaller@googlegroups.com>,
	USB list <linux-usb@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	stable <stable@vger.kernel.org>
Subject: Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
Date: Wed, 26 Sep 2018 04:22:08 -0400 (EDT)	[thread overview]
Message-ID: <1046309093.16165115.1537950128159.JavaMail.zimbra@redhat.com> (raw)
In-Reply-To: <Pine.LNX.4.44L0.1809251600150.1517-100000@iolanthe.rowland.org>

Hello, Alan,

> Now consider the case at hand: the call to usb_find_alt_setting() from
> check_ctrlrecip().  In this case ps->dev->actconfig being NULL doesn't
> indicate an error or a bug; it merely indicates that the user is trying
> to send a control request to a device which happens to be unconfigured,
> which is a perfectly valid thing to do.  Therefore it shouldn't require
> any special handling at the call site.
> 
> Alan Stern

Thank you for the explanation and a detailed response.

Best regards,
Vladis Dronov | Red Hat, Inc. | Product Security Engineer

----- Original Message -----
> From: "Alan Stern" <stern@rowland.harvard.edu>
> To: "Vladis Dronov" <vdronov@redhat.com>
> Cc: "Andrey Konovalov" <andreyknvl@google.com>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Oliver Neukum"
> <oneukum@suse.com>, "Hans de Goede" <hdegoede@redhat.com>, "syzkaller" <syzkaller@googlegroups.com>, "USB list"
> <linux-usb@vger.kernel.org>, "LKML" <linux-kernel@vger.kernel.org>, "stable" <stable@vger.kernel.org>
> Sent: Tuesday, September 25, 2018 10:44:14 PM
> Subject: Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt_setting()
> 
> On Tue, 25 Sep 2018, Vladis Dronov wrote:
> 
> > > > What about adding a WARN_ON()? It doesn't crash the kernel and it will
> > > > be detected and reported by syzbot.
> > 
> > Yes, that would be a great solution.
> > 
> > > Sure, we could do that.  But would be the point?
> > 
> > We know when usb_find_alt_setting() callers do smth weird and go fix them.
> > 
> > > After c9a4cb204e9e, calling usb_find_alt_setting() with a NULL config is
> > > no more of a bug than calling kfree() with a NULL pointer.
> > 
> > Yes, exactly.
> > 
> > > You wouldn't want to put a WARN_ON in kfree(), would you?
> > 
> > Honestly, in the ideal world I would, again, to be aware when some code
> > does
> > something weird so we know about it. But this world is this world, it needs
> > more performance to the throne of performance.
> 
> But is it really worthwhile?  In terms of catching bugs, this would
> help in only one situation: when the programmer thinks the argument
> should always be non-NULL because a NULL argument indicates a bug.
> Such situations seem to be relatively rare, and we can handle them by
> inserting a WARN_ON() at the call site if need be.
> 
> So it's a choice between:
> 
>      1. Putting a single test for NULL in the function being called,
> 	together with WARN_ON() at a small number of call sites, or
> 
>      2. Putting a WARN_ON() (or allowing a crash) in the function being
> 	called, together with tests for NULL at a potentially large
> 	number of call sites.
> 
> 1 has two advantages over 2.  First, it involves adding less code
> overall.  Second, it doesn't require the programmer to remember to add
> special code (a test or a WARN_ON) in situation where it doesn't
> matter -- presumably the majority of them.
> 