linux-hardening - Re: [PATCH V2] usb: musb: Check requset->buf before use to avoid crash issue

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Mon, 13 Nov 2023 19:50:06 +0800
From: xingxing luo <xingxing0070.luo@...il.com>
To: Greg KH <gregkh@...uxfoundation.org>
Cc: Xingxing Luo <xingxing.luo@...soc.com>, b-liu@...com, keescook@...omium.org, 
	nathan@...nel.org, ndesaulniers@...gle.com, trix@...hat.com, 
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-hardening@...r.kernel.org, llvm@...ts.linux.dev, Zhiyong.Liu@...soc.com, 
	Cixi.Geng1@...soc.com, Orson.Zhai@...soc.com, zhang.lyra@...il.com
Subject: Re: [PATCH V2] usb: musb: Check requset->buf before use to avoid
 crash issue

On Wed, Nov 1, 2023 at 3:31 PM Greg KH <gregkh@...uxfoundation.org> wrote:
>
> On Wed, Nov 01, 2023 at 03:14:21PM +0800, Xingxing Luo wrote:
> > When connecting USB to PC, there is a very low probability of kernel
> > crash. The reason is that in ep0_txstate(), the buf member of struct
> > usb_request used may be a null pointer. Therefore, it needs to
> > determine whether it is null before using it.
> >
> > [ 4888.071462][T597@C0] Call trace:
> > [ 4888.071467][T597@C0]  musb_default_write_fifo+0xa0/0x1ac [musb_hdrc]
> > [ 4888.087190][T597@C0]  musb_write_fifo+0x3c/0x90 [musb_hdrc]
> > [ 4888.099826][T597@C0]  ep0_txstate+0x78/0x218 [musb_hdrc]
> > [ 4888.153918][T597@C0]  musb_g_ep0_irq+0x3c4/0xe10 [musb_hdrc]
> > [ 4888.159663][T597@C0]  musb_interrupt+0xab4/0xf1c [musb_hdrc]
> > [ 4888.165391][T597@C0]  sprd_musb_interrupt+0x1e4/0x484 [musb_sprd]
> > [ 4888.171447][T597@C0]  __handle_irq_event_percpu+0xd8/0x2f8
> > [ 4888.176901][T597@C0]  handle_irq_event+0x70/0xe4
> > [ 4888.181487][T597@C0]  handle_fasteoi_irq+0x15c/0x230
> > [ 4888.186420][T597@C0]  handle_domain_irq+0x88/0xfc
> > [ 4888.191090][T597@C0]  gic_handle_irq+0x60/0x138
> > [ 4888.195591][T597@C0]  call_on_irq_stack+0x40/0x70
> > [ 4888.200263][T597@C0]  do_interrupt_handler+0x50/0xac
> > [ 4888.205196][T597@C0]  el1_interrupt+0x34/0x64
> > [ 4888.209524][T597@C0]  el1h_64_irq_handler+0x1c/0x2c
> > [ 4888.214370][T597@C0]  el1h_64_irq+0x7c/0x80
> > [ 4888.218525][T597@C0]  __check_heap_object+0x1ac/0x1fc
> > [ 4888.223544][T597@C0]  __check_object_size+0x10c/0x20c
> > [ 4888.228563][T597@C0]  simple_copy_to_iter+0x40/0x74
> > [ 4888.233410][T597@C0]  __skb_datagram_iter+0xa0/0x310
> > [ 4888.238343][T597@C0]  skb_copy_datagram_iter+0x44/0x110
> > [ 4888.243535][T597@C0]  netlink_recvmsg+0xdc/0x364
> > [ 4888.248123][T597@C0]  ____sys_recvmsg.llvm.16749613423860851707+0x358/0x6c0
> > [ 4888.255045][T597@C0]  ___sys_recvmsg+0xe0/0x1dc
> > [ 4888.259544][T597@C0]  __arm64_sys_recvmsg+0xc4/0x10c
> > [ 4888.264478][T597@C0]  invoke_syscall+0x6c/0x15c
> > [ 4888.268976][T597@C0]  el0_svc_common.llvm.12373701176611417606+0xd4/0x120
> > [ 4888.275726][T597@C0]  do_el0_svc+0x34/0xac
> > [ 4888.279795][T597@C0]  el0_svc+0x28/0x90
> > [ 4888.283603][T597@C0]  el0t_64_sync_handler+0x88/0xec
> > [ 4888.288548][T597@C0]  el0t_64_sync+0x1b4/0x1b8
> > [ 4888.292956][T597@C0] Code: 540002c3 53027ea8 aa1303e9 71000508 (b840452a)
> > [ 4888.299789][T597@C0] ---[ end trace 14a301b7253e83cc ]---
> >
> > Fixes: 550a7375fe72 ("USB: Add MUSB and TUSB support")
> > Signed-off-by: Xingxing Luo <xingxing.luo@...soc.com>
> > ---
> > v1 -> v2: - Fixed a spelling error
> >           - Add the fixed commit id
> >
> >  drivers/usb/musb/musb_gadget_ep0.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/musb/musb_gadget_ep0.c b/drivers/usb/musb/musb_gadget_ep0.c
> > index 6d7336727388..19eb7a5e1fdc 100644
> > --- a/drivers/usb/musb/musb_gadget_ep0.c
> > +++ b/drivers/usb/musb/musb_gadget_ep0.c
> > @@ -531,6 +531,11 @@ static void ep0_txstate(struct musb *musb)
> >
> >       request = &req->request;
> >
> > +     if (!request->buf) {
> > +             musb_dbg(musb, "request->buf is NULL");
>
> Why is this debug line needed?
>
> > +             return;
>
> Shouldn't we be reporting an error here somehow?
>

This is a good suggestion, I will add it in the next version.

> And why has this issue never been seen before in this driver?  This is a
> very old driver, with millions, if not billions, of working systems with
> it.  What caused this to suddenly start happening?
>

This problem occurs in a special scenario and is also a very
low-probability problem.
When our machine is used as a device and connected to the computer, due to some
unknown reason, the USB cable is not pulled out, but the USB driver is
notified to
pull out. Out, the gadget is processing the unbind process. When the
execution is
completed, request->buf is released and points to NULL. At the same
time, since the
USB is not physically disconnected at this time, the USB will receive
an interrupt and
use ep0 for control transmission. The request->buf of ep0 used at this
time has been
used in the gadget unbind process. It is released and points to NULL,
thus using a null
pointer, causing the kernel to crash. Therefore, we think it would be
better to determine
whether it is null before using request->buf.

> thanks,
>
> greg k-h