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 ]---
Signed-off-by: Xingxing Luo <[email protected]>
---
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..5d0629866128 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 (!requset->buf) {
+ musb_dbg(musb, "request->buf is NULL");
+ return;
+ }
+
/* load the data */
fifo_src = (u8 *) request->buf + request->actual;
fifo_count = min((unsigned) MUSB_EP0_FIFOSIZE,
--
2.17.1
On Mon, Oct 23, 2023 at 05:31:53PM +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 ]---
>
> Signed-off-by: Xingxing Luo <[email protected]>
> ---
> 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..5d0629866128 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 (!requset->buf) {
> + musb_dbg(musb, "request->buf is NULL");
> + return;
> + }
> +
> /* load the data */
> fifo_src = (u8 *) request->buf + request->actual;
> fifo_count = min((unsigned) MUSB_EP0_FIFOSIZE,
> --
> 2.17.1
This patch does not even build, how did you test it?
And what commit id does this fix?
thanks,
greg k-h
Hi Xingxing,
kernel test robot noticed the following build errors:
[auto build test ERROR on usb/usb-testing]
[also build test ERROR on usb/usb-next usb/usb-linus linus/master v6.6-rc7 next-20231027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xingxing-Luo/usb-musb-Check-requset-buf-before-use-to-avoid-crash-issue/20231023-173938
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link: https://lore.kernel.org/r/20231023093153.6748-1-xingxing.luo%40unisoc.com
patch subject: [PATCH] usb: musb: Check requset->buf before use to avoid crash issue
config: arm-davinci_all_defconfig (https://download.01.org/0day-ci/archive/20231028/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231028/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> drivers/usb/musb/musb_gadget_ep0.c:534:7: error: use of undeclared identifier 'requset'; did you mean 'request'?
534 | if (!requset->buf) {
| ^~~~~~~
| request
drivers/usb/musb/musb_gadget_ep0.c:521:22: note: 'request' declared here
521 | struct usb_request *request;
| ^
1 error generated.
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for PINCTRL_SINGLE
Depends on [n]: PINCTRL [=n] && OF [=y] && HAS_IOMEM [=y]
Selected by [y]:
- ARCH_DAVINCI [=y] && ARCH_MULTI_V5 [=y] && CPU_LITTLE_ENDIAN [=y]
vim +534 drivers/usb/musb/musb_gadget_ep0.c
510
511 /*
512 * transmitting to the host (IN), this code might be called from IRQ
513 * and from kernel thread.
514 *
515 * Context: caller holds controller lock
516 */
517 static void ep0_txstate(struct musb *musb)
518 {
519 void __iomem *regs = musb->control_ep->regs;
520 struct musb_request *req = next_ep0_request(musb);
521 struct usb_request *request;
522 u16 csr = MUSB_CSR0_TXPKTRDY;
523 u8 *fifo_src;
524 u8 fifo_count;
525
526 if (!req) {
527 /* WARN_ON(1); */
528 musb_dbg(musb, "odd; csr0 %04x", musb_readw(regs, MUSB_CSR0));
529 return;
530 }
531
532 request = &req->request;
533
> 534 if (!requset->buf) {
535 musb_dbg(musb, "request->buf is NULL");
536 return;
537 }
538
539 /* load the data */
540 fifo_src = (u8 *) request->buf + request->actual;
541 fifo_count = min((unsigned) MUSB_EP0_FIFOSIZE,
542 request->length - request->actual);
543 musb_write_fifo(&musb->endpoints[0], fifo_count, fifo_src);
544 request->actual += fifo_count;
545
546 /* update the flags */
547 if (fifo_count < MUSB_MAX_END0_PACKET
548 || (request->actual == request->length
549 && !request->zero)) {
550 musb->ep0_state = MUSB_EP0_STAGE_STATUSOUT;
551 csr |= MUSB_CSR0_P_DATAEND;
552 } else
553 request = NULL;
554
555 /* report completions as soon as the fifo's loaded; there's no
556 * win in waiting till this last packet gets acked. (other than
557 * very precise fault reporting, needed by USB TMC; possible with
558 * this hardware, but not usable from portable gadget drivers.)
559 */
560 if (request) {
561 musb->ackpend = csr;
562 musb_g_ep0_giveback(musb, request);
563 if (!musb->ackpend)
564 return;
565 musb->ackpend = 0;
566 }
567
568 /* send it out, triggering a "txpktrdy cleared" irq */
569 musb_ep_select(musb->mregs, 0);
570 musb_writew(regs, MUSB_CSR0, csr);
571 }
572
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, Oct 27, 2023 at 6:54 PM Greg KH <[email protected]> wrote:
>
> On Mon, Oct 23, 2023 at 05:31:53PM +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 ]---
> >
> > Signed-off-by: Xingxing Luo <[email protected]>
> > ---
> > 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..5d0629866128 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 (!requset->buf) {
> > + musb_dbg(musb, "request->buf is NULL");
> > + return;
> > + }
> > +
> > /* load the data */
> > fifo_src = (u8 *) request->buf + request->actual;
> > fifo_count = min((unsigned) MUSB_EP0_FIFOSIZE,
> > --
> > 2.17.1
>
> This patch does not even build, how did you test it?
>
I have verified this patch on our platform, which is unisoc, and it
can fix the kernel crash problem. However, it seems that there is a
spelling error in this patch, I will fix it in the next version.
> And what commit id does this fix?
commit id: 550a7375fe720924241f0eb76e4a5c1a3eb8c32f
>
> thanks,
>
> greg k-h