| Message ID | 20230316155356.13391-1-valthebrewer@yandex.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: gadget: net2280: fix NULL pointer dereference | expand |
On Thu, Mar 16, 2023 at 06:53:55PM +0300, Valery Zabrovsky wrote: > In net2280_free_request(): > If _ep is NULL, then ep is NULL and is dereferenced > while trying to produce an error message. How can that ever happen? How did you test and hit this? > The patch replaces dev_err() with pr_err() which works fine. That's not a good idea for driver code to use, sorry. > Found by Linux Verification Center (linuxtesting.org) with SVACE. As I have said before, your testing framework needs a lot more work. good luck! greg k-h
On Thu, Mar 16, 2023 at 05:19:35PM +0100, Greg Kroah-Hartman wrote: > On Thu, Mar 16, 2023 at 06:53:55PM +0300, Valery Zabrovsky wrote: > > In net2280_free_request(): > > If _ep is NULL, then ep is NULL and is dereferenced > > while trying to produce an error message. > > How can that ever happen? How did you test and hit this? > > > The patch replaces dev_err() with pr_err() which works fine. > > That's not a good idea for driver code to use, sorry. > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > As I have said before, your testing framework needs a lot more work. > > good luck! In situations like this, it might be better to remove the check entirely. If a driver does pass a NULL pointer, it will lead to an invalid pointer dereference which will certainly cause an oops and might very well crash the system. That would be a lot more noticeable than an error message hidden in a kernel log! Greg, is there any general policy about the need for sanity checks such as this one? Like, don't put them in whenever a failure would lead to an immediate fault which would be easy to track down? Alan Stern
On Thu, Mar 16, 2023 at 02:22:35PM -0400, Alan Stern wrote: > On Thu, Mar 16, 2023 at 05:19:35PM +0100, Greg Kroah-Hartman wrote: > > On Thu, Mar 16, 2023 at 06:53:55PM +0300, Valery Zabrovsky wrote: > > > In net2280_free_request(): > > > If _ep is NULL, then ep is NULL and is dereferenced > > > while trying to produce an error message. > > > > How can that ever happen? How did you test and hit this? > > > > > The patch replaces dev_err() with pr_err() which works fine. > > > > That's not a good idea for driver code to use, sorry. > > > > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > > > As I have said before, your testing framework needs a lot more work. > > > > good luck! > > In situations like this, it might be better to remove the check > entirely. If a driver does pass a NULL pointer, it will lead to an > invalid pointer dereference which will certainly cause an oops and might > very well crash the system. That would be a lot more noticeable than an > error message hidden in a kernel log! > > Greg, is there any general policy about the need for sanity checks such > as this one? Like, don't put them in whenever a failure would lead > to an immediate fault which would be easy to track down? That's the policy, no need to check if a caller is abusing the code and will find out instantly. Otherwise we end up checking on every function, and that way lies madness. thanks, greg k-h
diff --git a/drivers/usb/gadget/udc/net2280.c b/drivers/usb/gadget/udc/net2280.c index 1b929c519cd7..a027d1323993 100644 --- a/drivers/usb/gadget/udc/net2280.c +++ b/drivers/usb/gadget/udc/net2280.c @@ -584,8 +584,7 @@ static void net2280_free_request(struct usb_ep *_ep, struct usb_request *_req) ep = container_of(_ep, struct net2280_ep, ep); if (!_ep || !_req) { - dev_err(&ep->dev->pdev->dev, "%s: Invalid ep=%p or req=%p\n", - __func__, _ep, _req); + pr_err("%s: Invalid ep=%p or req=%p\n", __func__, _ep, _req); return; }
In net2280_free_request(): If _ep is NULL, then ep is NULL and is dereferenced while trying to produce an error message. The patch replaces dev_err() with pr_err() which works fine. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 9ceafcc2b3ad ("usb: gadget: net2280: print error in ep_ops error paths") Signed-off-by: Valery Zabrovsky <valthebrewer@yandex.ru> --- drivers/usb/gadget/udc/net2280.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)