| Message ID | 20221024094853.2877441-1-yulei.sh@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: aspeed: fix buffer overflow | expand |
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.
> 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 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);