When NCM is used with hosts like Windows PC, it is observed that there are multiple NTB's contained in one usb request giveback. Since the driver unwraps the obtained request data assuming only one NTB is present, we loose the subsequent NTB's present resulting in data loss.
Fix this by checking the parsed block length with the obtained data length in usb request and continue parsing after the last byte of current NTB.
Cc: stable@vger.kernel.org Reviewed-by: Maciej ��enczykowski maze@google.com Signed-off-by: Krishna Kurapati quic_kriskura@quicinc.com --- drivers/usb/gadget/function/f_ncm.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index feccf4c8cc4f..f00f051438ec 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -1156,7 +1156,8 @@ static int ncm_unwrap_ntb(struct gether *port, struct sk_buff_head *list) { struct f_ncm *ncm = func_to_ncm(&port->func); - __le16 *tmp = (void *) skb->data; + unsigned char *ntb_ptr = (void *) skb->data; + __le16 *tmp; unsigned index, index2; int ndp_index; unsigned dg_len, dg_len2; @@ -1169,6 +1170,10 @@ static int ncm_unwrap_ntb(struct gether *port, const struct ndp_parser_opts *opts = ncm->parser_opts; unsigned crc_len = ncm->is_crc ? sizeof(uint32_t) : 0; int dgram_counter; + int to_process = skb->len; + +parse_ntb: + tmp = (void *) ntb_ptr;
/* dwSignature */ if (get_unaligned_le32(tmp) != opts->nth_sign) { @@ -1215,7 +1220,7 @@ static int ncm_unwrap_ntb(struct gether *port, * walk through NDP * dwSignature */ - tmp = (void *)(skb->data + ndp_index); + tmp = (void *)(ntb_ptr + ndp_index); if (get_unaligned_le32(tmp) != ncm->ndp_sign) { INFO(port->func.config->cdev, "Wrong NDP SIGN\n"); goto err; @@ -1272,11 +1277,11 @@ static int ncm_unwrap_ntb(struct gether *port, if (ncm->is_crc) { uint32_t crc, crc2;
- crc = get_unaligned_le32(skb->data + + crc = get_unaligned_le32(ntb_ptr + index + dg_len - crc_len); crc2 = ~crc32_le(~0, - skb->data + index, + ntb_ptr + index, dg_len - crc_len); if (crc != crc2) { INFO(port->func.config->cdev, @@ -1303,7 +1308,7 @@ static int ncm_unwrap_ntb(struct gether *port, dg_len - crc_len); if (skb2 == NULL) goto err; - skb_put_data(skb2, skb->data + index, + skb_put_data(skb2, ntb_ptr + index, dg_len - crc_len);
skb_queue_tail(list, skb2); @@ -1316,10 +1321,17 @@ static int ncm_unwrap_ntb(struct gether *port, } while (ndp_len > 2 * (opts->dgram_item_len * 2)); } while (ndp_index);
- dev_consume_skb_any(skb); - VDBG(port->func.config->cdev, "Parsed NTB with %d frames\n", dgram_counter); + + to_process -= block_len; + if (to_process != 0) { + ntb_ptr = (unsigned char *) (ntb_ptr + block_len); + goto parse_ntb; + } + + dev_consume_skb_any(skb); + return 0; err: skb_queue_purge(list);
On Fri, Sep 15, 2023 at 11:39:48AM +0530, Krishna Kurapati wrote:
When NCM is used with hosts like Windows PC, it is observed that there are multiple NTB's contained in one usb request giveback. Since the driver unwraps the obtained request data assuming only one NTB is present, we loose the subsequent NTB's present resulting in data loss.
Fix this by checking the parsed block length with the obtained data length in usb request and continue parsing after the last byte of current NTB.
Cc: stable@vger.kernel.org
What commit id does this fix?
Reviewed-by: Maciej Żenczykowski maze@google.com Signed-off-by: Krishna Kurapati quic_kriskura@quicinc.com
drivers/usb/gadget/function/f_ncm.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index feccf4c8cc4f..f00f051438ec 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -1156,7 +1156,8 @@ static int ncm_unwrap_ntb(struct gether *port, struct sk_buff_head *list) { struct f_ncm *ncm = func_to_ncm(&port->func);
- __le16 *tmp = (void *) skb->data;
- unsigned char *ntb_ptr = (void *) skb->data;
Why persist with the extra ' ', didn't checkpatch complain about this?
And why the cast at all?
- __le16 *tmp; unsigned index, index2; int ndp_index; unsigned dg_len, dg_len2;
@@ -1169,6 +1170,10 @@ static int ncm_unwrap_ntb(struct gether *port, const struct ndp_parser_opts *opts = ncm->parser_opts; unsigned crc_len = ncm->is_crc ? sizeof(uint32_t) : 0; int dgram_counter;
- int to_process = skb->len;
+parse_ntb:
- tmp = (void *) ntb_ptr;
Again, no blank space please.
And why the cast?
thanks,
greg k-h
On 9/17/2023 1:34 PM, Greg Kroah-Hartman wrote:
Cc: stable@vger.kernel.org
What commit id does this fix?
Hi Greg,
This fixes the initial patch that added the driver: 9f6ce4240a2bf456402c15c06768059e5973f28c
Reviewed-by: Maciej Żenczykowski maze@google.com Signed-off-by: Krishna Kurapati quic_kriskura@quicinc.com
drivers/usb/gadget/function/f_ncm.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index feccf4c8cc4f..f00f051438ec 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -1156,7 +1156,8 @@ static int ncm_unwrap_ntb(struct gether *port, struct sk_buff_head *list) { struct f_ncm *ncm = func_to_ncm(&port->func);
- __le16 *tmp = (void *) skb->data;
- unsigned char *ntb_ptr = (void *) skb->data;
Why persist with the extra ' ', didn't checkpatch complain about this?
And why the cast at all?
My bad. I ran the checkpatch and got the following result:
kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/510/testncm/kernel$ ./scripts/checkpatch.pl --strict 0001-usb-gadget-ncm-Handle-decoding-of-multiple-NTB-s-in-.patch WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #12: unwraps the obtained request data assuming only one NTB is present, we loose
CHECK: No space is necessary after a cast #34: FILE: drivers/usb/gadget/function/f_ncm.c:1159: + unsigned char *ntb_ptr = (void *) skb->data;
CHECK: No space is necessary after a cast #46: FILE: drivers/usb/gadget/function/f_ncm.c:1176: + tmp = (void *) ntb_ptr;
CHECK: No space is necessary after a cast #93: FILE: drivers/usb/gadget/function/f_ncm.c:1329: + ntb_ptr = (unsigned char *) (ntb_ptr + block_len);
total: 0 errors, 1 warnings, 3 checks, 67 lines checked
I ignored the checks and saw only that errors are 0. Seems like I missed fixing the commit text wrapping to 75 chars (On line 12 it has 76 chars). Will fix it up in v3.
As per the cast, I initially didn't add any cast and saw that the code was not able to parse the dwSignature of the NTH and decoding of all packets was failing. Only when I added the cast, was the function able to decode all packets properly.
- __le16 *tmp; unsigned index, index2; int ndp_index; unsigned dg_len, dg_len2;
@@ -1169,6 +1170,10 @@ static int ncm_unwrap_ntb(struct gether *port, const struct ndp_parser_opts *opts = ncm->parser_opts; unsigned crc_len = ncm->is_crc ? sizeof(uint32_t) : 0; int dgram_counter;
- int to_process = skb->len;
+parse_ntb:
- tmp = (void *) ntb_ptr;
Again, no blank space please.
And why the cast?
the second cast here was just to be in sync with the original code; __le16 *tmp = (void *) skb->data;
I didn't try removing this and running the test. Will check if the second one is required or if decoding is proper without it or not.
Regards, Krishna,
On 9/18/2023 1:07 PM, Krishna Kurapati PSSNV wrote:
On 9/17/2023 1:34 PM, Greg Kroah-Hartman wrote:
Cc: stable@vger.kernel.org
What commit id does this fix?
Hi Greg,
This fixes the initial patch that added the driver: 9f6ce4240a2bf456402c15c06768059e5973f28c
Reviewed-by: Maciej Żenczykowski maze@google.com Signed-off-by: Krishna Kurapati quic_kriskura@quicinc.com
drivers/usb/gadget/function/f_ncm.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index feccf4c8cc4f..f00f051438ec 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -1156,7 +1156,8 @@ static int ncm_unwrap_ntb(struct gether *port, struct sk_buff_head *list) { struct f_ncm *ncm = func_to_ncm(&port->func); - __le16 *tmp = (void *) skb->data; + unsigned char *ntb_ptr = (void *) skb->data;
Why persist with the extra ' ', didn't checkpatch complain about this?
And why the cast at all?
My bad. I ran the checkpatch and got the following result:
kriskura@hu-kriskura-hyd:/local/mnt/workspace/krishna/510/testncm/kernel$ ./scripts/checkpatch.pl --strict 0001-usb-gadget-ncm-Handle-decoding-of-multiple-NTB-s-in-.patch WARNING: Prefer a maximum 75 chars per line (possible unwrapped commit description?) #12: unwraps the obtained request data assuming only one NTB is present, we loose
CHECK: No space is necessary after a cast #34: FILE: drivers/usb/gadget/function/f_ncm.c:1159: + unsigned char *ntb_ptr = (void *) skb->data;
CHECK: No space is necessary after a cast #46: FILE: drivers/usb/gadget/function/f_ncm.c:1176: + tmp = (void *) ntb_ptr;
CHECK: No space is necessary after a cast #93: FILE: drivers/usb/gadget/function/f_ncm.c:1329: + ntb_ptr = (unsigned char *) (ntb_ptr + block_len);
total: 0 errors, 1 warnings, 3 checks, 67 lines checked
I ignored the checks and saw only that errors are 0. Seems like I missed fixing the commit text wrapping to 75 chars (On line 12 it has 76 chars). Will fix it up in v3.
As per the cast, I initially didn't add any cast and saw that the code was not able to parse the dwSignature of the NTH and decoding of all packets was failing. Only when I added the cast, was the function able to decode all packets properly.
+ __le16 *tmp; unsigned index, index2; int ndp_index; unsigned dg_len, dg_len2; @@ -1169,6 +1170,10 @@ static int ncm_unwrap_ntb(struct gether *port, const struct ndp_parser_opts *opts = ncm->parser_opts; unsigned crc_len = ncm->is_crc ? sizeof(uint32_t) : 0; int dgram_counter; + int to_process = skb->len;
+parse_ntb: + tmp = (void *) ntb_ptr;
Again, no blank space please.
And why the cast?
the second cast here was just to be in sync with the original code; __le16 *tmp = (void *) skb->data;
I didn't try removing this and running the test. Will check if the second one is required or if decoding is proper without it or not.
Regards, Krishna,
Hi Greg,
I rechecked the code and I don't see any error if I remove the typecast for ntb_ptr. If I remove the typecast for tmp, I see build errors as ntb_ptr is unsigned char and tmp is __le16. I have pushed v3 removing the typecast for ntb_ptr and fixing the checkpatch errors. Thanks for pointing out that the typecast is not needed.
Regards, Krishna,
linux-stable-mirror@lists.linaro.org
-
Greg Kroah-Hartman -
Krishna Kurapati -
Krishna Kurapati PSSNV