linux-kernel - Re: [PATCH] usb: usbfs: fix crash in check_ctrlrecip()->usb_find_alt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Pine.LNX.4.44L0.1809251600150.1517-100000@iolanthe.rowland.org>
Date:   Tue, 25 Sep 2018 16:44:14 -0400 (EDT)
From:   Alan Stern <stern@...land.harvard.edu>
To:     Vladis Dronov <vdronov@...hat.com>
cc:     Andrey Konovalov <andreyknvl@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Oliver Neukum <oneukum@...e.com>,
        Hans de Goede <hdegoede@...hat.com>,
        syzkaller <syzkaller@...glegroups.com>,
        USB list <linux-usb@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        stable <stable@...r.kernel.org>
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.

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