| Message ID | 1658369754-87005-1-git-send-email-quic_qianyu@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/1] bus: mhi: host: Fix up null pointer access in mhi_irq_handler | expand |
On Thu, Jul 21, 2022 at 10:15:54AM +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> > --- > v1->v2: change dev_err to dev_dbg > > drivers/bus/mhi/host/main.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > index f3aef77a..25ea1f8 100644 > --- a/drivers/bus/mhi/host/main.c > +++ b/drivers/bus/mhi/host/main.c > @@ -430,12 +430,20 @@ 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; > It'd be good to add a comment here on why we are checking for the NULL pointer. Something like, "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." Thanks, Mani > + 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. >
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c index f3aef77a..25ea1f8 100644 --- a/drivers/bus/mhi/host/main.c +++ b/drivers/bus/mhi/host/main.c @@ -430,12 +430,20 @@ 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 (!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> --- v1->v2: change dev_err to dev_dbg drivers/bus/mhi/host/main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)