Message ID | 20230915061001.18884-1-quic_kriskura@quicinc.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:172:b0:3f2:4152:657d with SMTP id h50csp1006891vqi; Fri, 15 Sep 2023 05:30:45 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGO1i66yhpL5IwN80lzAdU7pUOfc33lNye3CYMRUr/RYc9XEARg4IUiy+L0AszUrWb+IrJU X-Received: by 2002:a17:90a:db8a:b0:268:798:a28b with SMTP id h10-20020a17090adb8a00b002680798a28bmr6528916pjv.23.1694781045082; Fri, 15 Sep 2023 05:30:45 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1694781045; cv=none; d=google.com; s=arc-20160816; b=CviWRh4G2MbjZJIII5ljsMmvAL6TA1Z5de/dLPwPqrrXQMU5h24kccChomsBnsBKXZ UDpRO6mKrWKruh4PLouZEupj10EX8yZ7gzKCcy8RfLhCUirdXWZ1pEAXNArKU2kV+DZ4 L2mawoalu4jfxvIKo/qzqwW2CniZ7wykem4QkZpsQDJC7PLOMp+J0ULWq/LZICgMqTEc 05K6MhnmCJLB4Tegf6HsTwSOKLhUlDHv16aKpi0cffnvs0JAOxyRnA+ddNWF+5JBkLhD 5n7uRh7E91nlFQvA4MFlNwd2nPkFGvTslLlmH7KY70kMFEvHcewDnAzt8r42KM43WhZw FSZg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=L/Y1nO9PHzxKa+60GpGl8/DXiZwZ5uTO9iL91cbIaDc=; fh=qt7f8R5sPQJF6tZ6SvtbfIVjZWRAkzRa23HOUj6Wz6Y=; b=IheVKhmSiOwk7DfJz7Kmh1WSZzar33I7ony865Wz6T4MQq+wD+KxNEnXgWqwe/jQBQ vy+fbsZR6OWL72majJSyllDQoAmiP8fKAnOHh7VZD73KDsqqvJzG/O6vFRIaHpKYexDh R4rGn8sVhs1a4RcyVdmBftU00BUuX5ZkFK8eBDLvDiI355xIJ4zVQesBQyaoPKiOeZm7 54getztpCoNpuTx8voDCLzl53FSrn848EJJDXGSKN3dRmY4M732k+WzL48u1nTTAilq4 p4jQIwFeoHPLB3K6QGfsAnbDJmiLxi5BTNnljsC5Mg6g0tbZTmtnZfdMtBmw0wauslqT xnYQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=KFxBCccV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from pete.vger.email (pete.vger.email. [2620:137:e000::3:6]) by mx.google.com with ESMTPS id s8-20020a17090a764800b00263f3d759cdsi3269443pjl.78.2023.09.15.05.30.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Sep 2023 05:30:45 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6; Authentication-Results: mx.google.com; dkim=pass header.i=@quicinc.com header.s=qcppdkim1 header.b=KFxBCccV; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=quicinc.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id 322F783ED021; Thu, 14 Sep 2023 23:10:39 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232215AbjIOGKZ (ORCPT <rfc822;pwkd43@gmail.com> + 33 others); Fri, 15 Sep 2023 02:10:25 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60180 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229554AbjIOGKY (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 15 Sep 2023 02:10:24 -0400 Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0CDBF1BD0; Thu, 14 Sep 2023 23:10:18 -0700 (PDT) Received: from pps.filterd (m0279870.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 38F5lLQr022421; Fri, 15 Sep 2023 06:10:17 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=quicinc.com; h=from : to : cc : subject : date : message-id : mime-version : content-type : content-transfer-encoding; s=qcppdkim1; bh=L/Y1nO9PHzxKa+60GpGl8/DXiZwZ5uTO9iL91cbIaDc=; b=KFxBCccVuwKueoiQUDLhMHsLaco6ZMWGf5Xz5hJgCMZf2A3S29YRbIxZokn7LfEoPdZJ eZgaBVQNYNGYKka+8ML8++vArzPL//jFA6ihM1LdjM2iT7cksGdrFwtXMPjPc4kRmLXt GbF/nDBxYmgUkFSsfj7jUs7dFULtLguNZD+qMVvQLEEUIPe16soqSB82Pw+W+JEwHY0b QB6tynnfBvqfrtXj5rgEpkC27qGAF5xev9j8MC2dqAFZLrdukfm+vWz3dmSD4Uelj8Fs /mWNobC4nJMbqZ1wwhznog6A1+6l98KWKkg0UHW7Fy7igWJ9MGZXzIcBnXmJag2Y37rv Tw== Received: from nalasppmta02.qualcomm.com (Global_NAT1.qualcomm.com [129.46.96.20]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3t4g86r6b3-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 15 Sep 2023 06:10:16 +0000 Received: from nalasex01a.na.qualcomm.com (nalasex01a.na.qualcomm.com [10.47.209.196]) by NALASPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 38F6AFAc009862 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 15 Sep 2023 06:10:15 GMT Received: from hu-kriskura-hyd.qualcomm.com (10.80.80.8) by nalasex01a.na.qualcomm.com (10.47.209.196) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.36; Thu, 14 Sep 2023 23:10:12 -0700 From: Krishna Kurapati <quic_kriskura@quicinc.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Linyu Yuan <quic_linyyuan@quicinc.com>, =?utf-8?q?Maciej_=C5=BBenczykowski?= <maze@google.com> CC: <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <quic_ppratap@quicinc.com>, <quic_wcheng@quicinc.com>, <quic_jackp@quicinc.com>, Krishna Kurapati <quic_kriskura@quicinc.com>, <stable@vger.kernel.org> Subject: [PATCH v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call Date: Fri, 15 Sep 2023 11:39:48 +0530 Message-ID: <20230915061001.18884-1-quic_kriskura@quicinc.com> X-Mailer: git-send-email 2.42.0 MIME-Version: 1.0 Content-Type: text/plain; charset="y" Content-Transfer-Encoding: 8bit X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nalasex01a.na.qualcomm.com (10.47.209.196) X-QCInternal: smtphost X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=5800 signatures=585085 X-Proofpoint-GUID: cLGA2U9WY19T-f71Sg8ZeCxizqz5QQfU X-Proofpoint-ORIG-GUID: cLGA2U9WY19T-f71Sg8ZeCxizqz5QQfU X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.267,Aquarius:18.0.980,Hydra:6.0.601,FMLib:17.11.176.26 definitions=2023-09-15_05,2023-09-14_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 clxscore=1011 bulkscore=0 impostorscore=0 adultscore=0 suspectscore=0 malwarescore=0 spamscore=0 mlxscore=0 priorityscore=1501 mlxlogscore=999 lowpriorityscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2308100000 definitions=main-2309150053 X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Thu, 14 Sep 2023 23:10:39 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777057584570738483 X-GMAIL-MSGID: 1777106729208994771 |
Series |
[v2] usb: gadget: ncm: Handle decoding of multiple NTB's in unwrap call
|
|
Commit Message
Krishna Kurapati
Sept. 15, 2023, 6:09 a.m. UTC
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(-)
Comments
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);