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@xxxxxxxxxxxxxxxxxxx> > To: "Vladis Dronov" <vdronov@xxxxxxxxxx> > Cc: "Andrey Konovalov" <andreyknvl@xxxxxxxxxx>, "Greg Kroah-Hartman" <gregkh@xxxxxxxxxxxxxxxxxxx>, "Oliver Neukum" > <oneukum@xxxxxxxx>, "Hans de Goede" <hdegoede@xxxxxxxxxx>, "syzkaller" <syzkaller@xxxxxxxxxxxxxxxx>, "USB list" > <linux-usb@xxxxxxxxxxxxxxx>, "LKML" <linux-kernel@xxxxxxxxxxxxxxx>, "stable" <stable@xxxxxxxxxxxxxxx> > 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. >
