[PATCH v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call

[PATCH v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call

[Krishna Kurapati]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="y", Size: 3069 bytes --]

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);
-- 
2.42.0

Re: [PATCH v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call

[Greg Kroah-Hartman]

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

Re: [PATCH v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call

[Krishna Kurapati PSSNV]

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,

Re: [PATCH v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call

[Krishna Kurapati PSSNV]

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,