| Message ID | 20200921135951.24045-1-johan@kernel.org |
|---|---|
| Headers | show |
| Series | USB: cdc-acm: handle broken union descriptors | expand |
Hi Johan, Yes, sure. Patching.... Cheers. --- Daniel
Hi Johan, Oliver, I just compiled and tested, everything still works. Cheers. --- Daniel
Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold: > > > interface? By hardcoding the data-interface number to be the one and > > > only interface, you'd end up probing for a "combined" interface also > > > with a broken call-management descriptor. > > > > Well, by the changelog assuming a combined interface caused an oops. > > Thence I am forced to conclude that the davices _has_ a separate > > data interface, but no union descriptor. > > No, the oops was probably due to the missing sanity check later added by > 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces"). > > With a broken call-management descriptor pointing to a non-existent > interface we'd oops before that commit. Hi, maybe I am dense, but a patch that comes after a patch that is said to fix something? Furthermore that patch would not come it work, it would merely make probe() fail cleanly. If I read the changelog correctly, the change makes the device work. Regards Oliver
On Mon, Sep 21, 2020 at 07:17:37PM +0200, Oliver Neukum wrote: > Am Montag, den 21.09.2020, 17:16 +0200 schrieb Johan Hovold: > > > > interface? By hardcoding the data-interface number to be the one and > > > > only interface, you'd end up probing for a "combined" interface also > > > > with a broken call-management descriptor. > > > > > > Well, by the changelog assuming a combined interface caused an oops. > > > Thence I am forced to conclude that the davices _has_ a separate > > > data interface, but no union descriptor. > > > > No, the oops was probably due to the missing sanity check later added by > > 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces"). > > > > With a broken call-management descriptor pointing to a non-existent > > interface we'd oops before that commit. > > Hi, > > maybe I am dense, but a patch that comes after a patch that is said to > fix something? Furthermore that patch would not come it work, > it would merely make probe() fail cleanly. If I read the changelog > correctly, the change makes the device work. The relevant commits are a2bfb4a346d2 ("USB: support for cdc-acm of single interface devices") (2009) fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") (2011) 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces") (2014) Before Greg added the sanity checks in that last commit, a broken call-management descriptor referring to a non-existent interface would cause a NULL-pointer dereference. The second commit, adding support for a specific device, didn't fix that problem generally and only worked around it for one device by hardcoding the data interface to match the control interface, thereby falling back to the "combined-interface" probing you added in that first commit. See? The second commit fixed the oops *and* made the device probe successfully. If we'd had the sanity check in place by then it would only have make it probe successfully. Johan
On Mon, Sep 21, 2020 at 06:19:12PM +0200, <Daniel Caujolle-Bert> wrote: > Hi Johan, Oliver, > > I just compiled and tested, everything still works. Thanks for testing, Daniel. If you want to you can reply to the second patch with a Tested-by tag so that it gets added by Greg's tooling (or reply to the cover letter if you want to have that tag added to all the patches in the series). Johan
Hi Johan, Okay, I replied to the second patch, hope I didn't make any mistake. Cheers. --- Daniel
On Tue, Sep 22, 2020 at 11:56:02AM +0200, <Daniel Caujolle-Bert> wrote: > Hi Johan, > > Okay, I replied to the second patch, hope I didn't make any mistake. Looks good. Thanks again! Johan
Am Dienstag, den 22.09.2020, 09:05 +0200 schrieb Johan Hovold: > The relevant commits are > > a2bfb4a346d2 ("USB: support for cdc-acm of single interface devices") (2009) > fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") (2011) > 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces") (2014) > > Before Greg added the sanity checks in that last commit, a broken > call-management descriptor referring to a non-existent interface would > cause a NULL-pointer dereference. Yes. > The second commit, adding support for a specific device, didn't fix that > problem generally Yes > and only worked around it for one device by hardcoding > the data interface to match the control interface, How do you know. It hardcoded the data interface. That it matches the control interface is a guess. > thereby falling back > to the "combined-interface" probing you added in that first commit. How do you know? They may or may not match. Regards Oliver
On Tue, Sep 22, 2020 at 12:40:42PM +0200, Oliver Neukum wrote: > Am Dienstag, den 22.09.2020, 09:05 +0200 schrieb Johan Hovold: > > > The relevant commits are > > > > a2bfb4a346d2 ("USB: support for cdc-acm of single interface devices") (2009) > > fd5054c169d2 ("USB: cdc_acm: Fix oops when Droids MuIn LCD is connected") (2011) > > 403dff4e2c94 ("USB: cdc-acm: check for valid interfaces") (2014) > > > > Before Greg added the sanity checks in that last commit, a broken > > call-management descriptor referring to a non-existent interface would > > cause a NULL-pointer dereference. > > Yes. > > > The second commit, adding support for a specific device, didn't fix that > > problem generally > > Yes > > > and only worked around it for one device by hardcoding > > the data interface to match the control interface, > > How do you know. It hardcoded the data interface. That it matches > the control interface is a guess. No, see below. > > thereby falling back > > to the "combined-interface" probing you added in that first commit. > > How do you know? They may or may not match. Heh. Did you actually read the commit message? "Add NO_DATA_INTERFACE quirk to tell the driver that "control" and "data" interfaces are not separated for this device, which prevents dereferencing a null pointer in the device probe code." Convinced yet? Johan
Am Dienstag, den 22.09.2020, 12:54 +0200 schrieb Johan Hovold: > Heh. Did you actually read the commit message? > > "Add NO_DATA_INTERFACE quirk to tell the driver that "control" > and "data" interfaces are not separated for this device, which > prevents dereferencing a null pointer in the device probe > code." > > Convinced yet? This patch does not fully match its description. Very well. Proceed. Could you resubmit and I'll ack? Regards Oliver
On Tue, Sep 22, 2020 at 01:41:06PM +0200, Oliver Neukum wrote: > Am Dienstag, den 22.09.2020, 12:54 +0200 schrieb Johan Hovold: > > > Heh. Did you actually read the commit message? > > > > "Add NO_DATA_INTERFACE quirk to tell the driver that "control" > > and "data" interfaces are not separated for this device, which > > prevents dereferencing a null pointer in the device probe > > code." > > > > Convinced yet? > > This patch does not fully match its description. Very well. Proceed. > Could you resubmit and I'll ack? I think you can just ack whole series by replying to 0/4. If Greg wants it resubmitted I'll include that ack when resending. Works for you? Johan
Am Montag, den 21.09.2020, 15:59 +0200 schrieb Johan Hovold: > This series adds support for handling broken union descriptors by > falling back to "combined-interface" probing. > > The first patch drops some bogus altsetting sanity checks which would > otherwise have had to be needlessly reproduced for consistency. The > third patch drops the driver specific data class define in favour of the > common one. The last one, cleans up the no-union-descriptor handling by > probing for a "combined-interface" before falling back to the > call-management descriptor. > > Note that I changed my mind on the stable tag; we can't be overly > paranoid about a theoretical risk of breaking some quirky devices. And > if we do, we still want to know about it, right? Acked-by: Oliver Neukum <oneukum@suse.com>