|
Takashi Iwai |
ed30ce |
From 2b405533c2560d7878199c57d95a39151351df72 Mon Sep 17 00:00:00 2001
|
|
Takashi Iwai |
ed30ce |
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
|
|
Takashi Iwai |
ed30ce |
Date: Sun, 20 Sep 2020 18:01:58 +0100
|
|
Takashi Iwai |
ed30ce |
Subject: [PATCH] USB: gadget: f_ncm: Fix NDP16 datagram validation
|
|
Denis Kirjanov |
fbb39b |
Git-commit: 028296e480c782f13428f234a8239a0cd007bd92
|
|
Denis Kirjanov |
fbb39b |
Patch-mainline: v5.10-rc1
|
|
Takashi Iwai |
ed30ce |
References: git-fixes
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
commit 2b74b0a04d3e ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()")
|
|
Takashi Iwai |
ed30ce |
adds important bounds checking however it unfortunately also introduces a
|
|
Takashi Iwai |
ed30ce |
bug with respect to section 3.3.1 of the NCM specification.
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
wDatagramIndex[1] : "Byte index, in little endian, of the second datagram
|
|
Takashi Iwai |
ed30ce |
described by this NDP16. If zero, then this marks the end of the sequence
|
|
Takashi Iwai |
ed30ce |
of datagrams in this NDP16."
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
Wdatagramlength[1]: "Byte length, in little endian, of the second datagram
|
|
Takashi Iwai |
ed30ce |
described by this NDP16. If zero, then this marks the end of the sequence
|
|
Takashi Iwai |
ed30ce |
of datagrams in this NDP16."
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
wDatagramIndex[1] and wDatagramLength[1] respectively then may be zero but
|
|
Takashi Iwai |
ed30ce |
that does not mean we should throw away the data referenced by
|
|
Takashi Iwai |
ed30ce |
wDatagramIndex[0] and wDatagramLength[0] as is currently the case.
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
Breaking the loop on (index2 == 0 || dg_len2 == 0) should come at the end
|
|
Takashi Iwai |
ed30ce |
as was previously the case and checks for index2 and dg_len2 should be
|
|
Takashi Iwai |
ed30ce |
removed since zero is valid.
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
I'm not sure how much testing the above patch received but for me right now
|
|
Takashi Iwai |
ed30ce |
after enumeration ping doesn't work. Reverting the commit restores ping,
|
|
Takashi Iwai |
ed30ce |
scp, etc.
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
The extra validation associated with wDatagramIndex[0] and
|
|
Takashi Iwai |
ed30ce |
wDatagramLength[0] appears to be valid so, this change removes the incorrect
|
|
Takashi Iwai |
ed30ce |
restriction on wDatagramIndex[1] and wDatagramLength[1] restoring data
|
|
Takashi Iwai |
ed30ce |
processing between host and device.
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
Fixes: 2b74b0a04d3e ("USB: gadget: f_ncm: add bounds checks to ncm_unwrap_ntb()")
|
|
Takashi Iwai |
ed30ce |
Cc: Ilja Van Sprundel <ivansprundel@ioactive.com>
|
|
Takashi Iwai |
ed30ce |
Cc: Brooke Basile <brookebasile@gmail.com>
|
|
Takashi Iwai |
ed30ce |
Cc: stable <stable@kernel.org>
|
|
Takashi Iwai |
ed30ce |
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
|
|
Takashi Iwai |
ed30ce |
Link: https://lore.kernel.org/r/20200920170158.1217068-1-bryan.odonoghue@linaro.org
|
|
Takashi Iwai |
ed30ce |
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
|
|
Takashi Iwai |
ed30ce |
Acked-by: Takashi Iwai <tiwai@suse.de>
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
---
|
|
Takashi Iwai |
ed30ce |
drivers/usb/gadget/function/f_ncm.c | 30 ++----------------------------
|
|
Takashi Iwai |
ed30ce |
1 file changed, 2 insertions(+), 28 deletions(-)
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c
|
|
Takashi Iwai |
ed30ce |
index b4206b0dede5..1f638759a953 100644
|
|
Takashi Iwai |
ed30ce |
--- a/drivers/usb/gadget/function/f_ncm.c
|
|
Takashi Iwai |
ed30ce |
+++ b/drivers/usb/gadget/function/f_ncm.c
|
|
Takashi Iwai |
ed30ce |
@@ -1189,7 +1189,6 @@ static int ncm_unwrap_ntb(struct gether *port,
|
|
Takashi Iwai |
ed30ce |
const struct ndp_parser_opts *opts = ncm->parser_opts;
|
|
Takashi Iwai |
ed30ce |
unsigned crc_len = ncm->is_crc ? sizeof(uint32_t) : 0;
|
|
Takashi Iwai |
ed30ce |
int dgram_counter;
|
|
Takashi Iwai |
ed30ce |
- bool ndp_after_header;
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
/* dwSignature */
|
|
Takashi Iwai |
ed30ce |
if (get_unaligned_le32(tmp) != opts->nth_sign) {
|
|
Takashi Iwai |
ed30ce |
@@ -1216,7 +1215,6 @@ static int ncm_unwrap_ntb(struct gether *port,
|
|
Takashi Iwai |
ed30ce |
}
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
ndp_index = get_ncm(&tmp, opts->ndp_index);
|
|
Takashi Iwai |
ed30ce |
- ndp_after_header = false;
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
/* Run through all the NDP's in the NTB */
|
|
Takashi Iwai |
ed30ce |
do {
|
|
Takashi Iwai |
ed30ce |
@@ -1232,8 +1230,6 @@ static int ncm_unwrap_ntb(struct gether *port,
|
|
Takashi Iwai |
ed30ce |
ndp_index);
|
|
Takashi Iwai |
ed30ce |
goto err;
|
|
Takashi Iwai |
ed30ce |
}
|
|
Takashi Iwai |
ed30ce |
- if (ndp_index == opts->nth_size)
|
|
Takashi Iwai |
ed30ce |
- ndp_after_header = true;
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
/*
|
|
Takashi Iwai |
ed30ce |
* walk through NDP
|
|
Takashi Iwai |
ed30ce |
@@ -1312,37 +1308,13 @@ static int ncm_unwrap_ntb(struct gether *port,
|
|
Takashi Iwai |
ed30ce |
index2 = get_ncm(&tmp, opts->dgram_item_len);
|
|
Takashi Iwai |
ed30ce |
dg_len2 = get_ncm(&tmp, opts->dgram_item_len);
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
- if (index2 == 0 || dg_len2 == 0)
|
|
Takashi Iwai |
ed30ce |
- break;
|
|
Takashi Iwai |
ed30ce |
-
|
|
Takashi Iwai |
ed30ce |
/* wDatagramIndex[1] */
|
|
Takashi Iwai |
ed30ce |
- if (ndp_after_header) {
|
|
Takashi Iwai |
ed30ce |
- if (index2 < opts->nth_size + opts->ndp_size) {
|
|
Takashi Iwai |
ed30ce |
- INFO(port->func.config->cdev,
|
|
Takashi Iwai |
ed30ce |
- "Bad index: %#X\n", index2);
|
|
Takashi Iwai |
ed30ce |
- goto err;
|
|
Takashi Iwai |
ed30ce |
- }
|
|
Takashi Iwai |
ed30ce |
- } else {
|
|
Takashi Iwai |
ed30ce |
- if (index2 < opts->nth_size + opts->dpe_size) {
|
|
Takashi Iwai |
ed30ce |
- INFO(port->func.config->cdev,
|
|
Takashi Iwai |
ed30ce |
- "Bad index: %#X\n", index2);
|
|
Takashi Iwai |
ed30ce |
- goto err;
|
|
Takashi Iwai |
ed30ce |
- }
|
|
Takashi Iwai |
ed30ce |
- }
|
|
Takashi Iwai |
ed30ce |
if (index2 > block_len - opts->dpe_size) {
|
|
Takashi Iwai |
ed30ce |
INFO(port->func.config->cdev,
|
|
Takashi Iwai |
ed30ce |
"Bad index: %#X\n", index2);
|
|
Takashi Iwai |
ed30ce |
goto err;
|
|
Takashi Iwai |
ed30ce |
}
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
- /* wDatagramLength[1] */
|
|
Takashi Iwai |
ed30ce |
- if ((dg_len2 < 14 + crc_len) ||
|
|
Takashi Iwai |
ed30ce |
- (dg_len2 > frame_max)) {
|
|
Takashi Iwai |
ed30ce |
- INFO(port->func.config->cdev,
|
|
Takashi Iwai |
ed30ce |
- "Bad dgram length: %#X\n", dg_len);
|
|
Takashi Iwai |
ed30ce |
- goto err;
|
|
Takashi Iwai |
ed30ce |
- }
|
|
Takashi Iwai |
ed30ce |
-
|
|
Takashi Iwai |
ed30ce |
/*
|
|
Takashi Iwai |
ed30ce |
* Copy the data into a new skb.
|
|
Takashi Iwai |
ed30ce |
* This ensures the truesize is correct
|
|
Takashi Iwai |
ed30ce |
@@ -1359,6 +1331,8 @@ static int ncm_unwrap_ntb(struct gether *port,
|
|
Takashi Iwai |
ed30ce |
ndp_len -= 2 * (opts->dgram_item_len * 2);
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
dgram_counter++;
|
|
Takashi Iwai |
ed30ce |
+ if (index2 == 0 || dg_len2 == 0)
|
|
Takashi Iwai |
ed30ce |
+ break;
|
|
Takashi Iwai |
ed30ce |
} while (ndp_len > 2 * (opts->dgram_item_len * 2));
|
|
Takashi Iwai |
ed30ce |
} while (ndp_index);
|
|
Takashi Iwai |
ed30ce |
|
|
Takashi Iwai |
ed30ce |
--
|
|
Takashi Iwai |
ed30ce |
2.16.4
|
|
Takashi Iwai |
ed30ce |
|