| Message ID | 20230915061001.18884-1-quic_kriskura@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call | expand |
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,
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);