diff mbox series

usb: gadget: aspeed: fix buffer overflow

Message ID 20221024094853.2877441-1-yulei.sh@bytedance.com
State New
Headers show
Series usb: gadget: aspeed: fix buffer overflow | expand

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

Benjamin Herrenschmidt Oct. 24, 2022, 9:59 p.m. UTC | #1
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.
Neal Liu Oct. 28, 2022, 6:59 a.m. UTC | #2
> 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
Lei Yu Oct. 28, 2022, 7:41 a.m. UTC | #3
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.
Neal Liu Oct. 28, 2022, 9:04 a.m. UTC | #4
> > 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.
Greg Kroah-Hartman Oct. 28, 2022, 9:17 a.m. UTC | #5
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
Neal Liu Oct. 28, 2022, 9:55 a.m. UTC | #6
> > > > 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.
Greg Kroah-Hartman Oct. 28, 2022, 10:45 a.m. UTC | #7
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
Lei Yu June 21, 2023, 12:02 p.m. UTC | #8
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?
Greg Kroah-Hartman June 21, 2023, 3:59 p.m. UTC | #9
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
Lei Yu June 25, 2023, 3:07 a.m. UTC | #10
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 mbox series

Patch

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