| Message ID | 1658459838-30802-1-git-send-email-quic_qianyu@quicinc.com |
|---|---|
| State | Accepted |
| Commit | acc5495bf764e7e441193f972d96c5d2887f6b8b |
| Headers | show |
| Series | [v3,1/1] bus: mhi: host: Fix up null pointer access in mhi_irq_handler | expand |
+ath11k, Kalle On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: > The irq handler for a shared IRQ ought to be prepared for running > even now it's being freed. So let's check the pointer used by > mhi_irq_handler to avoid null pointer access since it is probably > released before freeing IRQ. > > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> Thanks, Mani > --- > v2->v3: add comments > v1->v2: change dev_err to dev_dbg > > drivers/bus/mhi/host/main.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > index f3aef77a..df0fbfe 100644 > --- a/drivers/bus/mhi/host/main.c > +++ b/drivers/bus/mhi/host/main.c > @@ -430,12 +430,25 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) > { > struct mhi_event *mhi_event = dev; > struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl; > - struct mhi_event_ctxt *er_ctxt = > - &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; > + struct mhi_event_ctxt *er_ctxt; > struct mhi_ring *ev_ring = &mhi_event->ring; > - dma_addr_t ptr = le64_to_cpu(er_ctxt->rp); > + dma_addr_t ptr; > void *dev_rp; > > + /* > + * If CONFIG_DEBUG_SHIRQ is set, the IRQ handler will get invoked during __free_irq() > + * and by that time mhi_ctxt() would've freed. So check for the existence of mhi_ctxt > + * before handling the IRQs. > + */ > + if (!mhi_cntrl->mhi_ctxt) { > + dev_dbg(&mhi_cntrl->mhi_dev->dev, > + "mhi_ctxt has been freed\n"); > + return IRQ_HANDLED; > + } > + > + er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; > + ptr = le64_to_cpu(er_ctxt->rp); > + > if (!is_valid_ring_ptr(ev_ring, ptr)) { > dev_err(&mhi_cntrl->mhi_dev->dev, > "Event ring rp points outside of the event ring\n"); > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. >
Manivannan Sadhasivam <mani@kernel.org> writes: > +ath11k, Kalle > > On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: >> The irq handler for a shared IRQ ought to be prepared for running >> even now it's being freed. So let's check the pointer used by >> mhi_irq_handler to avoid null pointer access since it is probably >> released before freeing IRQ. >> >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> This fixes the crash and my regression tests pass now, thanks. But please see my question below. Tested-by: Kalle Valo <kvalo@kernel.org> >> + /* >> + * If CONFIG_DEBUG_SHIRQ is set, the IRQ handler will get invoked during __free_irq() >> + * and by that time mhi_ctxt() would've freed. So check for the existence of mhi_ctxt >> + * before handling the IRQs. >> + */ >> + if (!mhi_cntrl->mhi_ctxt) { >> + dev_dbg(&mhi_cntrl->mhi_dev->dev, >> + "mhi_ctxt has been freed\n"); >> + return IRQ_HANDLED; >> + } I don't see any protection accessing mhi_cntrl->mhi_ctxt, is this really free of race conditions?
On Tue, Jul 26, 2022 at 08:53:58PM +0300, Kalle Valo wrote: > Manivannan Sadhasivam <mani@kernel.org> writes: > > > +ath11k, Kalle > > > > On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: > >> The irq handler for a shared IRQ ought to be prepared for running > >> even now it's being freed. So let's check the pointer used by > >> mhi_irq_handler to avoid null pointer access since it is probably > >> released before freeing IRQ. > >> > >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> > > > > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > > This fixes the crash and my regression tests pass now, thanks. But > please see my question below. > > Tested-by: Kalle Valo <kvalo@kernel.org> > Thanks Kalle! > >> + /* > >> + * If CONFIG_DEBUG_SHIRQ is set, the IRQ handler will get invoked during __free_irq() > >> + * and by that time mhi_ctxt() would've freed. So check for the existence of mhi_ctxt > >> + * before handling the IRQs. > >> + */ > >> + if (!mhi_cntrl->mhi_ctxt) { > >> + dev_dbg(&mhi_cntrl->mhi_dev->dev, > >> + "mhi_ctxt has been freed\n"); > >> + return IRQ_HANDLED; > >> + } > > I don't see any protection accessing mhi_cntrl->mhi_ctxt, is this really > free of race conditions? > As Qiang said, it is safe to access mhi_ctxt here. Thanks, Mani > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: > The irq handler for a shared IRQ ought to be prepared for running > even now it's being freed. So let's check the pointer used by > mhi_irq_handler to avoid null pointer access since it is probably > released before freeing IRQ. > > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> Applied to mhi-next! This patch is targeted for v5.20-rcS. Thanks, Mani > --- > v2->v3: add comments > v1->v2: change dev_err to dev_dbg > > drivers/bus/mhi/host/main.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > index f3aef77a..df0fbfe 100644 > --- a/drivers/bus/mhi/host/main.c > +++ b/drivers/bus/mhi/host/main.c > @@ -430,12 +430,25 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) > { > struct mhi_event *mhi_event = dev; > struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl; > - struct mhi_event_ctxt *er_ctxt = > - &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; > + struct mhi_event_ctxt *er_ctxt; > struct mhi_ring *ev_ring = &mhi_event->ring; > - dma_addr_t ptr = le64_to_cpu(er_ctxt->rp); > + dma_addr_t ptr; > void *dev_rp; > > + /* > + * If CONFIG_DEBUG_SHIRQ is set, the IRQ handler will get invoked during __free_irq() > + * and by that time mhi_ctxt() would've freed. So check for the existence of mhi_ctxt > + * before handling the IRQs. > + */ > + if (!mhi_cntrl->mhi_ctxt) { > + dev_dbg(&mhi_cntrl->mhi_dev->dev, > + "mhi_ctxt has been freed\n"); > + return IRQ_HANDLED; > + } > + > + er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; > + ptr = le64_to_cpu(er_ctxt->rp); > + > if (!is_valid_ring_ptr(ev_ring, ptr)) { > dev_err(&mhi_cntrl->mhi_dev->dev, > "Event ring rp points outside of the event ring\n"); > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. >
Manivannan Sadhasivam <mani@kernel.org> writes: > On Tue, Jul 26, 2022 at 08:53:58PM +0300, Kalle Valo wrote: >> Manivannan Sadhasivam <mani@kernel.org> writes: >> >> > +ath11k, Kalle >> > >> > On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: >> >> The irq handler for a shared IRQ ought to be prepared for running >> >> even now it's being freed. So let's check the pointer used by >> >> mhi_irq_handler to avoid null pointer access since it is probably >> >> released before freeing IRQ. >> >> >> >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >> > >> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> >> >> This fixes the crash and my regression tests pass now, thanks. But >> please see my question below. >> >> Tested-by: Kalle Valo <kvalo@kernel.org> >> > > Thanks Kalle! I just tested v6.0-rc3 and it's still crashing. What's the plan to get this fix to Linus?
On Mon, Aug 29, 2022 at 07:20:10PM +0300, Kalle Valo wrote: > Manivannan Sadhasivam <mani@kernel.org> writes: > > > On Tue, Jul 26, 2022 at 08:53:58PM +0300, Kalle Valo wrote: > >> Manivannan Sadhasivam <mani@kernel.org> writes: > >> > >> > +ath11k, Kalle > >> > > >> > On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: > >> >> The irq handler for a shared IRQ ought to be prepared for running > >> >> even now it's being freed. So let's check the pointer used by > >> >> mhi_irq_handler to avoid null pointer access since it is probably > >> >> released before freeing IRQ. > >> >> > >> >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> > >> > > >> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> > >> > >> This fixes the crash and my regression tests pass now, thanks. But > >> please see my question below. > >> > >> Tested-by: Kalle Valo <kvalo@kernel.org> > >> > > > > Thanks Kalle! > > I just tested v6.0-rc3 and it's still crashing. What's the plan to get > this fix to Linus? Sorry for the delay. Just sent the PR to Greg for including it as a fix for v6.0. Thanks, Mani > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Manivannan Sadhasivam <mani@kernel.org> writes: > On Mon, Aug 29, 2022 at 07:20:10PM +0300, Kalle Valo wrote: >> Manivannan Sadhasivam <mani@kernel.org> writes: >> >> > On Tue, Jul 26, 2022 at 08:53:58PM +0300, Kalle Valo wrote: >> >> Manivannan Sadhasivam <mani@kernel.org> writes: >> >> >> >> > +ath11k, Kalle >> >> > >> >> > On Fri, Jul 22, 2022 at 11:17:18AM +0800, Qiang Yu wrote: >> >> >> The irq handler for a shared IRQ ought to be prepared for running >> >> >> even now it's being freed. So let's check the pointer used by >> >> >> mhi_irq_handler to avoid null pointer access since it is probably >> >> >> released before freeing IRQ. >> >> >> >> >> >> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> >> >> > >> >> > Reviewed-by: Manivannan Sadhasivam <mani@kernel.org> >> >> >> >> This fixes the crash and my regression tests pass now, thanks. But >> >> please see my question below. >> >> >> >> Tested-by: Kalle Valo <kvalo@kernel.org> >> >> >> > >> > Thanks Kalle! >> >> I just tested v6.0-rc3 and it's still crashing. What's the plan to get >> this fix to Linus? > > Sorry for the delay. Just sent the PR to Greg for including it as a fix for > v6.0. Excellent, thank you.
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c index f3aef77a..df0fbfe 100644 --- a/drivers/bus/mhi/host/main.c +++ b/drivers/bus/mhi/host/main.c @@ -430,12 +430,25 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) { struct mhi_event *mhi_event = dev; struct mhi_controller *mhi_cntrl = mhi_event->mhi_cntrl; - struct mhi_event_ctxt *er_ctxt = - &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; + struct mhi_event_ctxt *er_ctxt; struct mhi_ring *ev_ring = &mhi_event->ring; - dma_addr_t ptr = le64_to_cpu(er_ctxt->rp); + dma_addr_t ptr; void *dev_rp; + /* + * If CONFIG_DEBUG_SHIRQ is set, the IRQ handler will get invoked during __free_irq() + * and by that time mhi_ctxt() would've freed. So check for the existence of mhi_ctxt + * before handling the IRQs. + */ + if (!mhi_cntrl->mhi_ctxt) { + dev_dbg(&mhi_cntrl->mhi_dev->dev, + "mhi_ctxt has been freed\n"); + return IRQ_HANDLED; + } + + er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; + ptr = le64_to_cpu(er_ctxt->rp); + if (!is_valid_ring_ptr(ev_ring, ptr)) { dev_err(&mhi_cntrl->mhi_dev->dev, "Event ring rp points outside of the event ring\n");
The irq handler for a shared IRQ ought to be prepared for running even now it's being freed. So let's check the pointer used by mhi_irq_handler to avoid null pointer access since it is probably released before freeing IRQ. Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com> --- v2->v3: add comments v1->v2: change dev_err to dev_dbg drivers/bus/mhi/host/main.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)