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: [email protected]
Reviewed-by: Maciej Żenczykowski <[email protected]>
Signed-off-by: Krishna Kurapati <[email protected]>
---
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);
--
2.42.0
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: [email protected]
What commit id does this fix?
> Reviewed-by: Maciej Żenczykowski <[email protected]>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> 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: [email protected]
>
> What commit id does this fix?
>
Hi Greg,
This fixes the initial patch that added the driver:
9f6ce4240a2bf456402c15c06768059e5973f28c
>> Reviewed-by: Maciej Żenczykowski <[email protected]>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> 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: [email protected]
>>
>> What commit id does this fix?
>>
>
> Hi Greg,
>
> This fixes the initial patch that added the driver:
> 9f6ce4240a2bf456402c15c06768059e5973f28c
>
>>> Reviewed-by: Maciej Żenczykowski <[email protected]>
>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>> ---
>>> 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,