From mboxrd@z Thu Jan 1 00:00:00 1970 From: Johan Hovold Date: Tue, 21 Aug 2018 09:39:32 +0000 Subject: Re: [PATCH] USB: serial: io_ti: array underflow in edge_interrupt_callback() Message-Id: <20180821093932.GF14967@localhost> List-Id: References: <20180814090715.z4amswxtv5cwdria@kili.mountain> In-Reply-To: <20180814090715.z4amswxtv5cwdria@kili.mountain> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Dan Carpenter Cc: Johan Hovold , Greg Kroah-Hartman , linux-usb@vger.kernel.org, kernel-janitors@vger.kernel.org On Tue, Aug 14, 2018 at 12:07:15PM +0300, Dan Carpenter wrote: > A malicious USB device could set port_number to -3 and we would > underflow the edge_serial->serial->port[] array. Good catch! > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Dan Carpenter > > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c > index 6d1d6efa3055..fa2af18c6efe 100644 > --- a/drivers/usb/serial/io_ti.c > +++ b/drivers/usb/serial/io_ti.c > @@ -1629,7 +1629,7 @@ static void edge_interrupt_callback(struct urb *urb) > struct device *dev; > unsigned char *data = urb->transfer_buffer; > int length = urb->actual_length; > - int port_number; > + unsigned int port_number; I'd prefer fixing the macro to never return a negative port number in the first place instead of relying on conversion rules. These devices only come with one or two ports and, while the protocol documentation is hard to come by, it seems what we really want to do is just to check the 7th bit and thus change: -#define TIUMP_GET_PORT_FROM_CODE(c) (((c) >> 4) - 3) +#define TIUMP_GET_PORT_FROM_CODE(c) (((c) >> 6) & 0x01) I only have a one-port device to test with, but I can at least confirm that bits 0x30 are always set and that that's probably why subtraction was used instead of masking out the relevant bit (i.e. it happened to work). This also avoids error messages such as bad port number 4294967293 should the ignored bits (0xb0) have unexpected values (e.g. 0). > int function; > int retval; > __u8 lsr; Turns out we had the same issue in ti_usb_3410_5052 which is based on io_ti, but where a recent change converted the port macro to a static helper. In case you found this using your static checkers, you may want to look into why it didn't catch that one. I'll submit two fixes to address this as per above. Thanks, Johan From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: USB: serial: io_ti: array underflow in edge_interrupt_callback() From: Johan Hovold Message-Id: <20180821093932.GF14967@localhost> Date: Tue, 21 Aug 2018 11:39:32 +0200 To: Dan Carpenter Cc: Johan Hovold , Greg Kroah-Hartman , linux-usb@vger.kernel.org, kernel-janitors@vger.kernel.org List-ID: T24gVHVlLCBBdWcgMTQsIDIwMTggYXQgMTI6MDc6MTVQTSArMDMwMCwgRGFuIENhcnBlbnRlciB3 cm90ZToKPiBBIG1hbGljaW91cyBVU0IgZGV2aWNlIGNvdWxkIHNldCBwb3J0X251bWJlciB0byAt MyBhbmQgd2Ugd291bGQKPiB1bmRlcmZsb3cgdGhlIGVkZ2Vfc2VyaWFsLT5zZXJpYWwtPnBvcnRb XSBhcnJheS4KCkdvb2QgY2F0Y2ghCgo+IEZpeGVzOiAxZGExNzdlNGMzZjQgKCJMaW51eC0yLjYu MTItcmMyIikKPiBTaWduZWQtb2ZmLWJ5OiBEYW4gQ2FycGVudGVyIDxkYW4uY2FycGVudGVyQG9y YWNsZS5jb20+Cj4gCj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdXNiL3NlcmlhbC9pb190aS5jIGIv ZHJpdmVycy91c2Ivc2VyaWFsL2lvX3RpLmMKPiBpbmRleCA2ZDFkNmVmYTMwNTUuLmZhMmFmMThj NmVmZSAxMDA2NDQKPiAtLS0gYS9kcml2ZXJzL3VzYi9zZXJpYWwvaW9fdGkuYwo+ICsrKyBiL2Ry aXZlcnMvdXNiL3NlcmlhbC9pb190aS5jCj4gQEAgLTE2MjksNyArMTYyOSw3IEBAIHN0YXRpYyB2 b2lkIGVkZ2VfaW50ZXJydXB0X2NhbGxiYWNrKHN0cnVjdCB1cmIgKnVyYikKPiAgCXN0cnVjdCBk ZXZpY2UgKmRldjsKPiAgCXVuc2lnbmVkIGNoYXIgKmRhdGEgPSB1cmItPnRyYW5zZmVyX2J1ZmZl cjsKPiAgCWludCBsZW5ndGggPSB1cmItPmFjdHVhbF9sZW5ndGg7Cj4gLQlpbnQgcG9ydF9udW1i ZXI7Cj4gKwl1bnNpZ25lZCBpbnQgcG9ydF9udW1iZXI7CgpJJ2QgcHJlZmVyIGZpeGluZyB0aGUg bWFjcm8gdG8gbmV2ZXIgcmV0dXJuIGEgbmVnYXRpdmUgcG9ydCBudW1iZXIgaW4KdGhlIGZpcnN0 IHBsYWNlIGluc3RlYWQgb2YgcmVseWluZyBvbiBjb252ZXJzaW9uIHJ1bGVzLiBUaGVzZSBkZXZp Y2VzCm9ubHkgY29tZSB3aXRoIG9uZSBvciB0d28gcG9ydHMgYW5kLCB3aGlsZSB0aGUgcHJvdG9j b2wgZG9jdW1lbnRhdGlvbiBpcwpoYXJkIHRvIGNvbWUgYnksIGl0IHNlZW1zIHdoYXQgd2UgcmVh bGx5IHdhbnQgdG8gZG8gaXMganVzdCB0byBjaGVjayB0aGUKN3RoIGJpdCBhbmQgdGh1cyBjaGFu Z2U6CgotI2RlZmluZSBUSVVNUF9HRVRfUE9SVF9GUk9NX0NPREUoYykgICAgKCgoYykgPj4gNCkg LSAzKQorI2RlZmluZSBUSVVNUF9HRVRfUE9SVF9GUk9NX0NPREUoYykgICAgKCgoYykgPj4gNikg JiAweDAxKQoKSSBvbmx5IGhhdmUgYSBvbmUtcG9ydCBkZXZpY2UgdG8gdGVzdCB3aXRoLCBidXQg SSBjYW4gYXQgbGVhc3QgY29uZmlybQp0aGF0IGJpdHMgMHgzMCBhcmUgYWx3YXlzIHNldCBhbmQg dGhhdCB0aGF0J3MgcHJvYmFibHkgd2h5IHN1YnRyYWN0aW9uCndhcyB1c2VkIGluc3RlYWQgb2Yg bWFza2luZyBvdXQgdGhlIHJlbGV2YW50IGJpdCAoaS5lLiBpdCBoYXBwZW5lZCB0bwp3b3JrKS4K ClRoaXMgYWxzbyBhdm9pZHMgZXJyb3IgbWVzc2FnZXMgc3VjaCBhcwoKCWJhZCBwb3J0IG51bWJl ciA0Mjk0OTY3MjkzCgpzaG91bGQgdGhlIGlnbm9yZWQgYml0cyAoMHhiMCkgaGF2ZSB1bmV4cGVj dGVkIHZhbHVlcyAoZS5nLiAwKS4KCj4gIAlpbnQgZnVuY3Rpb247Cj4gIAlpbnQgcmV0dmFsOwo+ ICAJX191OCBsc3I7CgpUdXJucyBvdXQgd2UgaGFkIHRoZSBzYW1lIGlzc3VlIGluIHRpX3VzYl8z NDEwXzUwNTIgd2hpY2ggaXMgYmFzZWQgb24KaW9fdGksIGJ1dCB3aGVyZSBhIHJlY2VudCBjaGFu Z2UgY29udmVydGVkIHRoZSBwb3J0IG1hY3JvIHRvIGEgc3RhdGljCmhlbHBlci4gSW4gY2FzZSB5 b3UgZm91bmQgdGhpcyB1c2luZyB5b3VyIHN0YXRpYyBjaGVja2VycywgeW91IG1heSB3YW50CnRv IGxvb2sgaW50byB3aHkgaXQgZGlkbid0IGNhdGNoIHRoYXQgb25lLgoKSSdsbCBzdWJtaXQgdHdv IGZpeGVzIHRvIGFkZHJlc3MgdGhpcyBhcyBwZXIgYWJvdmUuCgpUaGFua3MsCkpvaGFuCg==