| Message ID | 20230926063015.21189-1-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call | expand |
On 9/26/2023 2:10 PM, Maciej Żenczykowski wrote: > On Tue, Sep 26, 2023 at 12:35 AM Krishna Kurapati PSSNV > <quic_kriskura@quicinc.com> wrote: >> On 9/26/2023 12:19 PM, Maciej Żenczykowski wrote: >>> On Mon, Sep 25, 2023 at 11:30 PM Krishna Kurapati >>> <quic_kriskura@quicinc.com> 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 >>>> Fixes: 9f6ce4240a2b ("usb: gadget: f_ncm.c added") >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>> --- >>>> Changes in v3: Removed explicit typecast for ntb_ptr >>>> >>>> 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 424bb3b666db..9512cec662c8 100644 >>>> --- a/drivers/usb/gadget/function/f_ncm.c >>>> +++ b/drivers/usb/gadget/function/f_ncm.c >>>> @@ -1171,7 +1171,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 = skb->data; >>>> + __le16 *tmp; >>>> unsigned index, index2; >>>> int ndp_index; >>>> unsigned dg_len, dg_len2; >>>> @@ -1184,6 +1185,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; >>> >>> Yeah this works, but really it should just be >>> tmp = (__le16 *)ntb_ptr; >>> >> >> Hi Maciej, >> >> Agree, it must be __le16* but I wanted to keep things similar to what >> was in original function. Do you want me to try with __le16* ? > > I think proper casts are usually better than void*... > Could you resend if it works? Compile test should be enough. > >> And I think you missed adding mailing list in CC. Can I add them on CC ? > > It wasn't intentional, normally gmail defaults to reply all, so not > sure what happened. > Go ahead. > Sure. Will test with __le16* and see if it works. If it does, will send it. And CC'ing linux-usb list as well on these replies. Thanks, Krishna, >> >> Regards, >> Krishna, >>>> >>>> /* dwSignature */ >>>> if (get_unaligned_le32(tmp) != opts->nth_sign) { >>>> @@ -1230,7 +1235,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); >>> ditto >>>> if (get_unaligned_le32(tmp) != ncm->ndp_sign) { >>>> INFO(port->func.config->cdev, "Wrong NDP SIGN\n"); >>>> goto err; >>>> @@ -1287,11 +1292,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, >>>> @@ -1318,7 +1323,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); >>>> @@ -1331,10 +1336,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) { >>> >>> you don't really need this != 0, but I guess that's just stylistic preference. >>> >>>> + 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.0Maciej Żenczykowski, Kernel Networking Developer @ Google
diff --git a/drivers/usb/gadget/function/f_ncm.c b/drivers/usb/gadget/function/f_ncm.c index 424bb3b666db..9512cec662c8 100644 --- a/drivers/usb/gadget/function/f_ncm.c +++ b/drivers/usb/gadget/function/f_ncm.c @@ -1171,7 +1171,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 = skb->data; + __le16 *tmp; unsigned index, index2; int ndp_index; unsigned dg_len, dg_len2; @@ -1184,6 +1185,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) { @@ -1230,7 +1235,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; @@ -1287,11 +1292,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, @@ -1318,7 +1323,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); @@ -1331,10 +1336,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);
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 Fixes: 9f6ce4240a2b ("usb: gadget: f_ncm.c added") Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- Changes in v3: Removed explicit typecast for ntb_ptr drivers/usb/gadget/function/f_ncm.c | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-)