[PATCH] usb: gadget: aspeed: fix buffer overflow

[PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei YU]

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(-)

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

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Benjamin Herrenschmidt]

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.

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei Yu]

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.

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Benjamin Herrenschmidt]

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.

RE: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Neal Liu]

> 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

RE: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei Yu]

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.

RE: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Neal Liu]

> > 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.

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Greg Kroah-Hartman]

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

RE: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Neal Liu]

> > > > 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.

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Greg Kroah-Hartman]

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

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei Yu]

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

RE: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Neal Liu]

> > 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

RE: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei Yu]

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.

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei Yu]

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?

Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Greg Kroah-Hartman]

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

Re: [External] Re: [PATCH] usb: gadget: aspeed: fix buffer overflow

[Lei Yu]

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.