| Message ID | 20231023093153.6748-1-xingxing.luo@unisoc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: musb: Check requset->buf before use to avoid crash issue | expand |
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 <xingxing.luo@unisoc.com> > --- > 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/202310282331.d4wx1Z6b-lkp@intel.com/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/202310282331.d4wx1Z6b-lkp@intel.com/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 <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202310282331.d4wx1Z6b-lkp@intel.com/ 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
On Fri, Oct 27, 2023 at 6:54 PM Greg KH <gregkh@linuxfoundation.org> 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 <xingxing.luo@unisoc.com> > > --- > > 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
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,
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 <xingxing.luo@unisoc.com> --- drivers/usb/musb/musb_gadget_ep0.c | 5 +++++ 1 file changed, 5 insertions(+)