Message ID | 20221024094853.2877441-1-yulei.sh@bytedance.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp355841wru; Mon, 24 Oct 2022 03:02:43 -0700 (PDT) X-Google-Smtp-Source: AMsMyM7A1n6owXXMjzQo51wa8nHCu4ag01AJTRNRyTgyAtaimJ2wO0YyViGZ1oYpOXYZaxuskE7e X-Received: by 2002:a17:902:f60c:b0:17c:163f:c0a8 with SMTP id n12-20020a170902f60c00b0017c163fc0a8mr31713938plg.38.1666605763607; Mon, 24 Oct 2022 03:02:43 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666605763; cv=none; d=google.com; s=arc-20160816; b=oMhzxWdxyrcfoiUMqN6fgcbyXRINQpY1x1da6+4eDzdl8APAMhwoukneFgWoXqqBGt eSDlbDTZfcwho6teMMbEfzDmNPvxAWWSbhsy/5k4oaEauwq0XL0t3oew5A3h3NqLNvpp c93cTirLYQk/Ug3J1ENjngtHM+a2f5Af+31ALTShNeaGMhSTYDFUQEoUTt/u6kXSgNYB xMsQMPPxgP3H8zDbD+vNLILTCJc9fJBzLIcSc/Ckz5VtVS6SYBWFtvC9yrtLSWLKDzhX wwK1gIBj9vyp7eLvzJxRlPMYROnPvJnCH9ESQGX2X4MS/+6sdDR1BQDONCM5jYNTRokw z9Rw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:message-id:date:subject:to:from:dkim-signature; bh=XnAGFFH6hlB9GkUsVAF0QK5SQgC3hf7SewwhsiTdnHA=; b=CtY7EQlS/W5Oxy5UUvqhvT+Kt1Vmn5pZ3w3XVFk3+76zbB027vauQRSgnSttLimre1 dbLXSDW9hLC7j6M4npibMIS64mwAkhy5nY9EKenbmsCvxvPdWngdIPLautTT+8RR7KA1 DzSMWelN7GgnzYrgqH47fioggbD8Q6KJ/2P93epuXFRdBeNLqhztjRzRd0ab/6W8FCcj U5yADLmh+3f9g2O0lU3ujqysW6Nkr9mj5ZdTRe+21apz8ZADPYmTzPVrH0JMBvxGI74N 5ppqzQyb/PbBFCiMx9sl1cvm4SjvRPWHMTE1bvNORhAX/ox7m2G3MoyHYAKnqmUuzu8d k/SQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=5kjelKIa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=bytedance.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s187-20020a632cc4000000b0045e96393e37si33671721pgs.20.2022.10.24.03.02.29; Mon, 24 Oct 2022 03:02:43 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=5kjelKIa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=bytedance.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230309AbiJXJtH (ORCPT <rfc822;pwkd43@gmail.com> + 99 others); Mon, 24 Oct 2022 05:49:07 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35036 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230016AbiJXJtF (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Mon, 24 Oct 2022 05:49:05 -0400 Received: from mail-pf1-x42e.google.com (mail-pf1-x42e.google.com [IPv6:2607:f8b0:4864:20::42e]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 443D560495 for <linux-kernel@vger.kernel.org>; Mon, 24 Oct 2022 02:48:59 -0700 (PDT) Received: by mail-pf1-x42e.google.com with SMTP id f140so8557179pfa.1 for <linux-kernel@vger.kernel.org>; Mon, 24 Oct 2022 02:48:59 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20210112.gappssmtp.com; s=20210112; h=message-id:date:subject:to:from:from:to:cc:subject:date:message-id :reply-to; bh=XnAGFFH6hlB9GkUsVAF0QK5SQgC3hf7SewwhsiTdnHA=; b=5kjelKIaoQNlKa+/O/o1rvz6jyKf6B3J8qwQv7aZhfvbh8k0GyIkH5SOcN2anjY7Ax J8s4LN2F2vS0tcd/9WiqQEXXYJINxsEgNo/NSHRFHNx//7ZGecdbOc7YWviJ6LnZVRsF uo+Hf5KMVgLFcFwTyyc4jAED5DvhQsI9ZDdbmGF/tg08oMiXUltHu8V73hvJ4RwSgil3 8eZUqK6P5QEWc2DDKGNb0UAaLu5NmK2x0DyI6kqv9d9RZbclUKwuiruh7nuUov5+/trk pLWSTn4ST0wxX7/r0DeAg8KjnefevIOYTSfptQ9OZwWr3fXDhBoks9jqt8B5W+i9Fnhy GO+A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XnAGFFH6hlB9GkUsVAF0QK5SQgC3hf7SewwhsiTdnHA=; b=Uj2YfGcPAcM+M9KxkjuymAjjQLxHHsgBCy3KJ95TpIqiQyHMNEUHSlDeCnNII6jVkp 6Uj26FzWZrhjhHq1A6L0hlc5bpknpbugA2OpEy+pfqasogr5O/kxllAoCi+F1ZTsDBrX 2Enu2qFbgYJCq4QJ4HCmerpcNBxF4xAS3l7s7/EYroktwbB4GJcT97dSGovT3K7qEGa9 dFePzuptJKMyqeM5UBA2sPp3asTiwZ9Kv2hqVwqq8ScOqLz59rJDHdnEQ892Y9wfk6Vj cUStggyAAP8+qlxBHmWj4GxFFbUJw2O1/MPeab5024XK1KNBv1+W8GHPMc0AmVGF9jI7 Cb4w== X-Gm-Message-State: ACrzQf1KoMHcGj8Z5qswTN6LaVs/QgTb/zvPSDsqJylJAwYiRuYQhQDr nWrauJPM+H2VgkKYLnHKPWPrMYCV26aYpQfZ X-Received: by 2002:a63:34c8:0:b0:46e:f67c:c117 with SMTP id b191-20020a6334c8000000b0046ef67cc117mr5823331pga.401.1666604938609; Mon, 24 Oct 2022 02:48:58 -0700 (PDT) Received: from localhost ([103.136.220.142]) by smtp.gmail.com with ESMTPSA id p1-20020a62d001000000b0056bc742d21esm1764058pfg.176.2022.10.24.02.48.57 (version=TLS1_2 cipher=ECDHE-ECDSA-CHACHA20-POLY1305 bits=256/256); Mon, 24 Oct 2022 02:48:58 -0700 (PDT) From: Lei YU <yulei.sh@bytedance.com> To: Felipe Balbi <balbi@kernel.org>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Joel Stanley <joel@jms.id.au>, Andrew Jeffery <andrew@aj.id.au>, Lei YU <yulei.sh@bytedance.com>, Henry Tian <tianxiaofeng@bytedance.com>, Jakob Koschel <jakobkoschel@gmail.com>, linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-aspeed@lists.ozlabs.org, linux-kernel@vger.kernel.org Subject: [PATCH] usb: gadget: aspeed: fix buffer overflow Date: Mon, 24 Oct 2022 09:48:53 +0000 Message-Id: <20221024094853.2877441-1-yulei.sh@bytedance.com> X-Mailer: git-send-email 2.11.0 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,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 lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747562805399918405?= X-GMAIL-MSGID: =?utf-8?q?1747562805399918405?= |
Series |
usb: gadget: aspeed: fix buffer overflow
|
|
Commit Message
Lei Yu
Oct. 24, 2022, 9:48 a.m. UTC
From: Henry Tian <tianxiaofeng@bytedance.com> In ast_vhub_epn_handle_ack() when the received data length exceeds the buffer, it does not check the case and just copies to req.buf and cause a buffer overflow, kernel oops on this case. This issue could be reproduced on a BMC with an OS that enables the lan over USB: 1. In OS, enable the usb eth dev, verify it pings the BMC OK; 2. In OS, set the usb dev mtu to 2000. (Default is 1500); 3. In OS, ping the BMC with `-s 2000` argument. The BMC kernel will get oops with below logs: skbuff: skb_over_panic: text:8058e098 len:2048 put:2048 head:84c678a0 data:84c678c2 tail:0x84c680c2 end:0x84c67f00 dev:usb0 ------------[ cut here ]------------ kernel BUG at net/core/skbuff.c:113! Internal error: Oops - BUG: 0 [#1] ARM CPU: 0 PID: 0 Comm: swapper Not tainted 5.15.69-c9fb275-dirty-d1e579a #1 Hardware name: Generic DT based system PC is at skb_panic+0x60/0x6c LR is at irq_work_queue+0x6c/0x94 Fix the issue by checking the length and set `-EOVERFLOW`. Tested: Verify the BMC kernel does not get oops in the above case, and the usb ethernet gets RX packets errors instead. Signed-off-by: Lei YU <yulei.sh@bytedance.com> Signed-off-by: Henry Tian <tianxiaofeng@bytedance.com> --- drivers/usb/gadget/udc/aspeed-vhub/core.c | 2 +- drivers/usb/gadget/udc/aspeed-vhub/epn.c | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-)
Comments
On Mon, 2022-10-24 at 09:48 +0000, Lei YU wrote: > From: Henry Tian <tianxiaofeng@bytedance.com> I wrote that driver, please CC me on further patches to it (thanks Joel for the heads up). > In ast_vhub_epn_handle_ack() when the received data length exceeds the > buffer, it does not check the case and just copies to req.buf and cause > a buffer overflow, kernel oops on this case. .../... Thanks ! Seems like a legit bug, however: > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c > index b5252880b389..56e55472daa1 100644 > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c > @@ -84,6 +84,7 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) > { > struct ast_vhub_req *req; > unsigned int len; > + int status = 0; > u32 stat; > > /* Read EP status */ > @@ -119,9 +120,15 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) > len = VHUB_EP_DMA_TX_SIZE(stat); > > /* If not using DMA, copy data out if needed */ > - if (!req->req.dma && !ep->epn.is_in && len) > - memcpy(req->req.buf + req->req.actual, ep->buf, len); > - > + if (!req->req.dma && !ep->epn.is_in && len) { > + if (req->req.actual + len > req->req.length) { > + req->last_desc = 1; > + status = -EOVERFLOW; > + goto done; Should we stall as well ? Should we continue receiving and just dropping the data until we have a small packet ? Otherwise the EP could get out of sync for subsequent ones... Additionally, I'm curious, why in this specific case is the device sending more data than the buffer can hold ? The MTU change should have resulted in buffers being re-allocated no ? Or did you change the MTU on the remote and not on the local device ? Cheers, Ben.
On Tue, Oct 25, 2022 at 6:00 AM Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > > On Mon, 2022-10-24 at 09:48 +0000, Lei YU wrote: > > From: Henry Tian <tianxiaofeng@bytedance.com> > > I wrote that driver, please CC me on further patches to it (thanks Joel > for the heads up). > > > In ast_vhub_epn_handle_ack() when the received data length exceeds the > > buffer, it does not check the case and just copies to req.buf and cause > > a buffer overflow, kernel oops on this case. > > .../... > > Thanks ! Seems like a legit bug, however: > > > diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c > > index b5252880b389..56e55472daa1 100644 > > --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c > > +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c > > @@ -84,6 +84,7 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) > > { > > struct ast_vhub_req *req; > > unsigned int len; > > + int status = 0; > > u32 stat; > > > > /* Read EP status */ > > @@ -119,9 +120,15 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) > > len = VHUB_EP_DMA_TX_SIZE(stat); > > > > /* If not using DMA, copy data out if needed */ > > - if (!req->req.dma && !ep->epn.is_in && len) > > - memcpy(req->req.buf + req->req.actual, ep->buf, len); > > - > > + if (!req->req.dma && !ep->epn.is_in && len) { > > + if (req->req.actual + len > req->req.length) { > > + req->last_desc = 1; > > + status = -EOVERFLOW; > > + goto done; > > Should we stall as well ? Should we continue receiving and just dropping the data until we have > a small packet ? Otherwise the EP could get out of sync for subsequent ones... > This case is treated as an error and we do not care about the following data. Similarly, if we change the MTU in BMC and let BMC ping the OS, the OS kernel does not crash and it gets RX errors, and the ping fails. # ifconfig usb0 usb0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 ... RX packets 85 bytes 15380 (15.0 KiB) RX errors 51 dropped 0 overruns 0 frame 51 With this patch, we get the similar behavior on BMC that the RX errors are increasing. > Additionally, I'm curious, why in this specific case is the device sending more data than > the buffer can hold ? The MTU change should have resulted in buffers being re-allocated no ? The issue is found in a rare case during BIOS boot, we assume that BIOS is sending unexpected data to BMC for unknown reasons. > Or did you change the MTU on the remote and not on the local device ? > Yes, the MTU is changed to 2000 in OS and kept 1500 on BMC, then the issue is reproduced. (see detailed steps in the above email). The reason we made the above test is because we are trying to reproduce the behavior as BIOS, and from the logs it looks like it's sending a packet larger than MTU. Then we tried to adjust the MTU on the OS side and reproduced the issue.
On Tue, 2022-10-25 at 14:21 +0800, Lei Yu wrote: > > This case is treated as an error and we do not care about the > following data. > Similarly, if we change the MTU in BMC and let BMC ping the OS, the > OS > kernel does not crash and it gets RX errors, and the ping fails. > > # ifconfig usb0 > usb0: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500 > ... > RX packets 85 bytes 15380 (15.0 KiB) > RX errors 51 dropped 0 overruns 0 frame 51 > > With this patch, we get the similar behavior on BMC that the RX > errors > are increasing. > > > Additionally, I'm curious, why in this specific case is the device > > sending more data than > > the buffer can hold ? The MTU change should have resulted in > > buffers being re-allocated no ? > > The issue is found in a rare case during BIOS boot, we assume that > BIOS is sending unexpected data to BMC for unknown reasons. Ok thanks. Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > > Or did you change the MTU on the remote and not on the local device > > ? > > > > Yes, the MTU is changed to 2000 in OS and kept 1500 on BMC, then the > issue is reproduced. (see detailed steps in the above email). > > The reason we made the above test is because we are trying to > reproduce the behavior as BIOS, and from the logs it looks like it's > sending a packet larger than MTU. Then we tried to adjust the MTU on > the OS side and reproduced the issue.
> From: Henry Tian <tianxiaofeng@bytedance.com> > > In ast_vhub_epn_handle_ack() when the received data length exceeds the > buffer, it does not check the case and just copies to req.buf and cause a buffer > overflow, kernel oops on this case. > > This issue could be reproduced on a BMC with an OS that enables the lan over > USB: > 1. In OS, enable the usb eth dev, verify it pings the BMC OK; 2. In OS, set the > usb dev mtu to 2000. (Default is 1500); 3. In OS, ping the BMC with `-s 2000` > argument. > > The BMC kernel will get oops with below logs: > > skbuff: skb_over_panic: text:8058e098 len:2048 put:2048 > head:84c678a0 data:84c678c2 tail:0x84c680c2 end:0x84c67f00 dev:usb0 > ------------[ cut here ]------------ > kernel BUG at net/core/skbuff.c:113! > Internal error: Oops - BUG: 0 [#1] ARM > CPU: 0 PID: 0 Comm: swapper Not tainted > 5.15.69-c9fb275-dirty-d1e579a #1 > Hardware name: Generic DT based system > PC is at skb_panic+0x60/0x6c > LR is at irq_work_queue+0x6c/0x94 > > Fix the issue by checking the length and set `-EOVERFLOW`. > > Tested: Verify the BMC kernel does not get oops in the above case, and the usb > ethernet gets RX packets errors instead. Thanks for your feedback. I tried to reproduce it on my side, and it cannot be reproduce it. Here are my test sequences: 1. emulate one of the vhub port to usb ethernet through Linux gadget (ncm) 2. connect BMC vhub to Host 3. BMC & Host can ping each other (both usb eth dev default mtu is 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu to 2000, it's maxmtu is 1500) 5. ping BMC with `s -1500` argument from Host OS 6. BMC kernel no oops I dumped the `req` related members in ast_vhub_epn_handle_ack() to see if whether the received data length exceeds the buffer length. In my case `req.length` is 16384 bytes, so it never exceeds it in this case. I'm wondering what's the value of `req.length` in your test scenario? And how can I reproduce it? Thanks
On Fri, Oct 28, 2022 at 2:59 PM Neal Liu <neal_liu@aspeedtech.com> wrote: > > Thanks for your feedback. > I tried to reproduce it on my side, and it cannot be reproduce it. > Here are my test sequences: > 1. emulate one of the vhub port to usb ethernet through Linux gadget (ncm) We are using rndis instead of ncm. > 2. connect BMC vhub to Host > 3. BMC & Host can ping each other (both usb eth dev default mtu is 1500) > 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu to 2000, it's maxmtu is 1500) Not sure if it's related, but in my case (USB rndis, Debian 10 OS) it should be able to set MTU to 2000. > 5. ping BMC with `s -1500` argument from Host OS > 6. BMC kernel no oops > > I dumped the `req` related members in ast_vhub_epn_handle_ack() to see if whether the received data length exceeds the buffer length. > In my case `req.length` is 16384 bytes, so it never exceeds it in this case. > I'm wondering what's the value of `req.length` in your test scenario? And how can I reproduce it? The last 3 calls of ast_vhub_epn_handle_ack(): ast_vhub_epn_handle_ack:req->last_desc=-1 req.actual=1024,req.length=1578,ep->ep.maxpacket=512 ast_vhub_epn_handle_ack:req->last_desc=-1 req.actual=1536,req.length=1578,ep->ep.maxpacket=512 ast_vhub_epn_handle_ack:req->last_desc=1 req.actual=1634,req.length=1578,ep->ep.maxpacket=512 We can see the last packet 1634 exceeds the req.legnth 1578, and that's when the buffer overflow occurs.
> > Thanks for your feedback. > > I tried to reproduce it on my side, and it cannot be reproduce it. > > Here are my test sequences: > > 1. emulate one of the vhub port to usb ethernet through Linux gadget > > (ncm) > > We are using rndis instead of ncm. > > > 2. connect BMC vhub to Host > > 3. BMC & Host can ping each other (both usb eth dev default mtu is > > 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu to > > 2000, it's maxmtu is 1500) > > Not sure if it's related, but in my case (USB rndis, Debian 10 OS) it should be > able to set MTU to 2000. Using rndis is able to set MTU to 2000, and the issue can be reproduced. Using ncm & ecm has no this issue because buffer length is more bigger than the received data length. (FYI) Thanks. Reviewed-by: Neal Liu <neal_liu@aspeedtech.com> > > > 5. ping BMC with `s -1500` argument from Host OS 6. BMC kernel no oops > > > > I dumped the `req` related members in ast_vhub_epn_handle_ack() to see if > whether the received data length exceeds the buffer length. > > In my case `req.length` is 16384 bytes, so it never exceeds it in this case. > > I'm wondering what's the value of `req.length` in your test scenario? And > how can I reproduce it? > > The last 3 calls of ast_vhub_epn_handle_ack(): > > ast_vhub_epn_handle_ack:req->last_desc=-1 > req.actual=1024,req.length=1578,ep->ep.maxpacket=512 > ast_vhub_epn_handle_ack:req->last_desc=-1 > req.actual=1536,req.length=1578,ep->ep.maxpacket=512 > ast_vhub_epn_handle_ack:req->last_desc=1 > req.actual=1634,req.length=1578,ep->ep.maxpacket=512 > > We can see the last packet 1634 exceeds the req.legnth 1578, and that's when > the buffer overflow occurs.
On Fri, Oct 28, 2022 at 09:04:52AM +0000, Neal Liu wrote: > > > Thanks for your feedback. > > > I tried to reproduce it on my side, and it cannot be reproduce it. > > > Here are my test sequences: > > > 1. emulate one of the vhub port to usb ethernet through Linux gadget > > > (ncm) > > > > We are using rndis instead of ncm. > > > > > 2. connect BMC vhub to Host > > > 3. BMC & Host can ping each other (both usb eth dev default mtu is > > > 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu to > > > 2000, it's maxmtu is 1500) > > > > Not sure if it's related, but in my case (USB rndis, Debian 10 OS) it should be > > able to set MTU to 2000. > > Using rndis is able to set MTU to 2000, and the issue can be reproduced. Please NEVER use rndis anymore. I need to go just delete that driver from the tree. It is insecure-by-design and will cause any system that runs it to be instantly compromised and it can not be fixed. Never trust it. Even for data throughput tests, I wouldn't trust it as it does odd things with packet sizes as you show here. thanks, greg k-h
> > > > Thanks for your feedback. > > > > I tried to reproduce it on my side, and it cannot be reproduce it. > > > > Here are my test sequences: > > > > 1. emulate one of the vhub port to usb ethernet through Linux > > > > gadget > > > > (ncm) > > > > > > We are using rndis instead of ncm. > > > > > > > 2. connect BMC vhub to Host > > > > 3. BMC & Host can ping each other (both usb eth dev default mtu is > > > > 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu > > > > to 2000, it's maxmtu is 1500) > > > > > > Not sure if it's related, but in my case (USB rndis, Debian 10 OS) > > > it should be able to set MTU to 2000. > > > > Using rndis is able to set MTU to 2000, and the issue can be reproduced. > > Please NEVER use rndis anymore. I need to go just delete that driver from > the tree. > > It is insecure-by-design and will cause any system that runs it to be instantly > compromised and it can not be fixed. Never trust it. > > Even for data throughput tests, I wouldn't trust it as it does odd things with > packet sizes as you show here. Thanks for the info, Greg. If rndis will no longer be supported, how to use usb-ethernet on Windows OS? For my understanding, ncm/ecm cannot work on Windows OS.
On Fri, Oct 28, 2022 at 09:55:57AM +0000, Neal Liu wrote: > > > > > Thanks for your feedback. > > > > > I tried to reproduce it on my side, and it cannot be reproduce it. > > > > > Here are my test sequences: > > > > > 1. emulate one of the vhub port to usb ethernet through Linux > > > > > gadget > > > > > (ncm) > > > > > > > > We are using rndis instead of ncm. > > > > > > > > > 2. connect BMC vhub to Host > > > > > 3. BMC & Host can ping each other (both usb eth dev default mtu is > > > > > 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu > > > > > to 2000, it's maxmtu is 1500) > > > > > > > > Not sure if it's related, but in my case (USB rndis, Debian 10 OS) > > > > it should be able to set MTU to 2000. > > > > > > Using rndis is able to set MTU to 2000, and the issue can be reproduced. > > > > Please NEVER use rndis anymore. I need to go just delete that driver from > > the tree. > > > > It is insecure-by-design and will cause any system that runs it to be instantly > > compromised and it can not be fixed. Never trust it. > > > > Even for data throughput tests, I wouldn't trust it as it does odd things with > > packet sizes as you show here. > > Thanks for the info, Greg. > If rndis will no longer be supported, how to use usb-ethernet on Windows OS? > For my understanding, ncm/ecm cannot work on Windows OS. rndis should ONLY be there for Windows XP, which is long out-of-support. Newer versions of windows have more sane usb protocols built into it and this driver is not needed. As proof of this, Android devices removed this from their kernel configuration a few years ago and no one has complained :) thanks, greg k-h
On Fri, Oct 28, 2022 at 6:45 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Fri, Oct 28, 2022 at 09:55:57AM +0000, Neal Liu wrote: > > > > > > Thanks for your feedback. > > > > > > I tried to reproduce it on my side, and it cannot be reproduce it. > > > > > > Here are my test sequences: > > > > > > 1. emulate one of the vhub port to usb ethernet through Linux > > > > > > gadget > > > > > > (ncm) > > > > > > > > > > We are using rndis instead of ncm. > > > > > > > > > > > 2. connect BMC vhub to Host > > > > > > 3. BMC & Host can ping each other (both usb eth dev default mtu is > > > > > > 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev mtu > > > > > > to 2000, it's maxmtu is 1500) > > > > > > > > > > Not sure if it's related, but in my case (USB rndis, Debian 10 OS) > > > > > it should be able to set MTU to 2000. > > > > > > > > Using rndis is able to set MTU to 2000, and the issue can be reproduced. USB ecm is also tested and it is possible to set MTU to 2000, and could reproduce the issue. So I think this patch is needed anyway. @Neal Liu Could you kindly help to verify the USB ECM case? > > > > > > Please NEVER use rndis anymore. I need to go just delete that driver from > > > the tree. > > > > > > It is insecure-by-design and will cause any system that runs it to be instantly > > > compromised and it can not be fixed. Never trust it. > > > > > > Even for data throughput tests, I wouldn't trust it as it does odd things with > > > packet sizes as you show here. > > > > Thanks for the info, Greg. > > If rndis will no longer be supported, how to use usb-ethernet on Windows OS? > > For my understanding, ncm/ecm cannot work on Windows OS. > > rndis should ONLY be there for Windows XP, which is long out-of-support. > Newer versions of windows have more sane usb protocols built into it and > this driver is not needed. > > As proof of this, Android devices removed this from their kernel > configuration a few years ago and no one has complained :) > > thanks, > > greg k-h
> > On Fri, Oct 28, 2022 at 09:55:57AM +0000, Neal Liu wrote: > > > > > > > Thanks for your feedback. > > > > > > > I tried to reproduce it on my side, and it cannot be reproduce it. > > > > > > > Here are my test sequences: > > > > > > > 1. emulate one of the vhub port to usb ethernet through > > > > > > > Linux gadget > > > > > > > (ncm) > > > > > > > > > > > > We are using rndis instead of ncm. > > > > > > > > > > > > > 2. connect BMC vhub to Host > > > > > > > 3. BMC & Host can ping each other (both usb eth dev default > > > > > > > mtu is > > > > > > > 1500) 4. Set BMC mtu to 1000 (Host OS cannot set usb eth dev > > > > > > > mtu to 2000, it's maxmtu is 1500) > > > > > > > > > > > > Not sure if it's related, but in my case (USB rndis, Debian 10 > > > > > > OS) it should be able to set MTU to 2000. > > > > > > > > > > Using rndis is able to set MTU to 2000, and the issue can be > reproduced. > > USB ecm is also tested and it is possible to set MTU to 2000, and could > reproduce the issue. > So I think this patch is needed anyway. > > @Neal Liu Could you kindly help to verify the USB ECM case? How to set MTU to 2000 on USB ECM case? I remember last time I cannot set by using "ifconfig ..." Regardless ECM or RNDIS, I agree this patch is still needed. > > > > > > > > Please NEVER use rndis anymore. I need to go just delete that > > > > driver from the tree. > > > > > > > > It is insecure-by-design and will cause any system that runs it to > > > > be instantly compromised and it can not be fixed. Never trust it. > > > > > > > > Even for data throughput tests, I wouldn't trust it as it does odd > > > > things with packet sizes as you show here. > > > > > > Thanks for the info, Greg. > > > If rndis will no longer be supported, how to use usb-ethernet on Windows > OS? > > > For my understanding, ncm/ecm cannot work on Windows OS. > > > > rndis should ONLY be there for Windows XP, which is long out-of-support. > > Newer versions of windows have more sane usb protocols built into it > > and this driver is not needed. > > > > As proof of this, Android devices removed this from their kernel > > configuration a few years ago and no one has complained :) > > > > thanks, > > > > greg k-h
On Wed, Dec 21, 2022 at 10:17 AM Neal Liu <neal_liu@aspeedtech.com> wrote: > > > > > > Using rndis is able to set MTU to 2000, and the issue can be > > reproduced. > > > > USB ecm is also tested and it is possible to set MTU to 2000, and could > > reproduce the issue. > > So I think this patch is needed anyway. > > > > @Neal Liu Could you kindly help to verify the USB ECM case? > > How to set MTU to 2000 on USB ECM case? I remember last time I cannot set by using "ifconfig ..." > Regardless ECM or RNDIS, I agree this patch is still needed. You were able to set MTU to 2000 for RNDIS but not for NCM. @Greg Kroah-Hartman indicated that RNDIS should not be used anymore. So I tested ECM and verified it could set MTU 2000 and the issue could be reproduced.
On Wed, Dec 21, 2022 at 10:26 AM Lei Yu <yulei.sh@bytedance.com> wrote: > > On Wed, Dec 21, 2022 at 10:17 AM Neal Liu <neal_liu@aspeedtech.com> wrote: > > > > > > > Using rndis is able to set MTU to 2000, and the issue can be > > > reproduced. > > > > > > USB ecm is also tested and it is possible to set MTU to 2000, and could > > > reproduce the issue. > > > So I think this patch is needed anyway. > > > > > > @Neal Liu Could you kindly help to verify the USB ECM case? > > > > How to set MTU to 2000 on USB ECM case? I remember last time I cannot set by using "ifconfig ..." > > Regardless ECM or RNDIS, I agree this patch is still needed. > > You were able to set MTU to 2000 for RNDIS but not for NCM. > @Greg Kroah-Hartman indicated that RNDIS should not be used anymore. > So I tested ECM and verified it could set MTU 2000 and the issue could > be reproduced. This patch fixes the kernel oops in the aspeed-vhub driver for both USB ECM and RNDIS. It now has an Acked-by from benh and Reviewed-by from neal_liu Should we merge this patch?
On Wed, Jun 21, 2023 at 08:02:14PM +0800, Lei Yu wrote: > On Wed, Dec 21, 2022 at 10:26 AM Lei Yu <yulei.sh@bytedance.com> wrote: > > > > On Wed, Dec 21, 2022 at 10:17 AM Neal Liu <neal_liu@aspeedtech.com> wrote: > > > > > > > > Using rndis is able to set MTU to 2000, and the issue can be > > > > reproduced. > > > > > > > > USB ecm is also tested and it is possible to set MTU to 2000, and could > > > > reproduce the issue. > > > > So I think this patch is needed anyway. > > > > > > > > @Neal Liu Could you kindly help to verify the USB ECM case? > > > > > > How to set MTU to 2000 on USB ECM case? I remember last time I cannot set by using "ifconfig ..." > > > Regardless ECM or RNDIS, I agree this patch is still needed. > > > > You were able to set MTU to 2000 for RNDIS but not for NCM. > > @Greg Kroah-Hartman indicated that RNDIS should not be used anymore. > > So I tested ECM and verified it could set MTU 2000 and the issue could > > be reproduced. > > This patch fixes the kernel oops in the aspeed-vhub driver for both > USB ECM and RNDIS. > It now has an Acked-by from benh and Reviewed-by from neal_liu > > Should we merge this patch? Can you please resend it? thanks, greg k-h
On Thu, Jun 22, 2023 at 12:00 AM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Wed, Jun 21, 2023 at 08:02:14PM +0800, Lei Yu wrote: > > On Wed, Dec 21, 2022 at 10:26 AM Lei Yu <yulei.sh@bytedance.com> wrote: > > > > > > On Wed, Dec 21, 2022 at 10:17 AM Neal Liu <neal_liu@aspeedtech.com> wrote: > > > > > > > > > Using rndis is able to set MTU to 2000, and the issue can be > > > > > reproduced. > > > > > > > > > > USB ecm is also tested and it is possible to set MTU to 2000, and could > > > > > reproduce the issue. > > > > > So I think this patch is needed anyway. > > > > > > > > > > @Neal Liu Could you kindly help to verify the USB ECM case? > > > > > > > > How to set MTU to 2000 on USB ECM case? I remember last time I cannot set by using "ifconfig ..." > > > > Regardless ECM or RNDIS, I agree this patch is still needed. > > > > > > You were able to set MTU to 2000 for RNDIS but not for NCM. > > > @Greg Kroah-Hartman indicated that RNDIS should not be used anymore. > > > So I tested ECM and verified it could set MTU 2000 and the issue could > > > be reproduced. > > > > This patch fixes the kernel oops in the aspeed-vhub driver for both > > USB ECM and RNDIS. > > It now has an Acked-by from benh and Reviewed-by from neal_liu > > > > Should we merge this patch? > > Can you please resend it? I just find that the patch is already merged in v6.2 Thanks.
diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c index 7a635c499777..ac3ca24f8b04 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c @@ -37,7 +37,7 @@ void ast_vhub_done(struct ast_vhub_ep *ep, struct ast_vhub_req *req, list_del_init(&req->queue); - if (req->req.status == -EINPROGRESS) + if ((req->req.status == -EINPROGRESS) || (status == -EOVERFLOW)) req->req.status = status; if (req->req.dma) { diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c b/drivers/usb/gadget/udc/aspeed-vhub/epn.c index b5252880b389..56e55472daa1 100644 --- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c +++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c @@ -84,6 +84,7 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) { struct ast_vhub_req *req; unsigned int len; + int status = 0; u32 stat; /* Read EP status */ @@ -119,9 +120,15 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) len = VHUB_EP_DMA_TX_SIZE(stat); /* If not using DMA, copy data out if needed */ - if (!req->req.dma && !ep->epn.is_in && len) - memcpy(req->req.buf + req->req.actual, ep->buf, len); - + if (!req->req.dma && !ep->epn.is_in && len) { + if (req->req.actual + len > req->req.length) { + req->last_desc = 1; + status = -EOVERFLOW; + goto done; + } else { + memcpy(req->req.buf + req->req.actual, ep->buf, len); + } + } /* Adjust size */ req->req.actual += len; @@ -129,9 +136,10 @@ static void ast_vhub_epn_handle_ack(struct ast_vhub_ep *ep) if (len < ep->ep.maxpacket) req->last_desc = 1; +done: /* That's it ? complete the request and pick a new one */ if (req->last_desc >= 0) { - ast_vhub_done(ep, req, 0); + ast_vhub_done(ep, req, status); req = list_first_entry_or_null(&ep->queue, struct ast_vhub_req, queue);