| Message ID | 1613578822-18614-1-git-send-email-jhugo@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] bus: mhi: core: Sanity check values from remote device before use | expand |
On Wed, Feb 17, 2021 at 09:20:22AM -0700, Jeffrey Hugo wrote: > When parsing the structures in the shared memory, there are values which > come from the remote device. For example, a transfer completion event > will have a pointer to the tre in the relevant channel's transfer ring. > Such values should be considered to be untrusted, and validated before > use. If we blindly use such values, we may access invalid data or crash > if the values are corrupted. > > If validation fails, drop the relevant event. > > Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> > --- > > v2: Fix subject > > drivers/bus/mhi/core/main.c | 81 +++++++++++++++++++++++++++++++++++++++++---- > 1 file changed, 74 insertions(+), 7 deletions(-) > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > index c043574..1eb2fd3 100644 > --- a/drivers/bus/mhi/core/main.c > +++ b/drivers/bus/mhi/core/main.c > @@ -242,6 +242,11 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, > smp_wmb(); > } > > +static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) > +{ > + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; > +} > + > int mhi_destroy_device(struct device *dev, void *data) > { > struct mhi_device *mhi_dev; > @@ -383,7 +388,16 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) > struct mhi_event_ctxt *er_ctxt = > &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; > struct mhi_ring *ev_ring = &mhi_event->ring; > - void *dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); > + dma_addr_t ptr = er_ctxt->rp; > + void *dev_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"); > + return IRQ_HANDLED; > + } > + > + dev_rp = mhi_to_virtual(ev_ring, ptr); > > /* Only proceed if event ring has pending events */ > if (ev_ring->rp == dev_rp) > @@ -536,6 +550,11 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, > struct mhi_buf_info *buf_info; > u16 xfer_len; > > + if (!is_valid_ring_ptr(tre_ring, ptr)) { > + dev_err(&mhi_cntrl->mhi_dev->dev, > + "Event element points outside of the tre ring\n"); > + break; > + } > /* Get the TRB this event points to */ > ev_tre = mhi_to_virtual(tre_ring, ptr); > > @@ -695,6 +714,12 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, > struct mhi_chan *mhi_chan; > u32 chan; > > + if (!is_valid_ring_ptr(mhi_ring, ptr)) { > + dev_err(&mhi_cntrl->mhi_dev->dev, > + "Event element points outside of the cmd ring\n"); > + return; > + } > + > cmd_pkt = mhi_to_virtual(mhi_ring, ptr); > > chan = MHI_TRE_GET_CMD_CHID(cmd_pkt); > @@ -719,6 +744,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > struct device *dev = &mhi_cntrl->mhi_dev->dev; > u32 chan; > int count = 0; > + dma_addr_t ptr = er_ctxt->rp; > > /* > * This is a quick check to avoid unnecessary event processing > @@ -728,7 +754,13 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) > return -EIO; > > - dev_rp = mhi_to_virtual(ev_ring, 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"); > + return -EIO; > + } > + > + dev_rp = mhi_to_virtual(ev_ring, ptr); > local_rp = ev_ring->rp; > > while (dev_rp != local_rp) { > @@ -834,6 +866,8 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > */ > if (chan < mhi_cntrl->max_chan) { > mhi_chan = &mhi_cntrl->mhi_chan[chan]; > + if (!mhi_chan->configured) > + break; This change is not part of this patch I believe. > parse_xfer_event(mhi_cntrl, local_rp, mhi_chan); > event_quota--; > } > @@ -845,7 +879,15 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > > mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring); > local_rp = ev_ring->rp; > - dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); > + > + ptr = 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"); > + return -EIO; > + } > + > + dev_rp = mhi_to_virtual(ev_ring, ptr); > count++; > } > > @@ -868,11 +910,18 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, > int count = 0; > u32 chan; > struct mhi_chan *mhi_chan; > + dma_addr_t ptr = er_ctxt->rp; > > if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) > return -EIO; > > - dev_rp = mhi_to_virtual(ev_ring, 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"); > + return -EIO; > + } > + > + dev_rp = mhi_to_virtual(ev_ring, ptr); > local_rp = ev_ring->rp; > > while (dev_rp != local_rp && event_quota > 0) { > @@ -886,7 +935,8 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, > * Only process the event ring elements whose channel > * ID is within the maximum supported range. > */ > - if (chan < mhi_cntrl->max_chan) { > + if (chan < mhi_cntrl->max_chan && > + mhi_cntrl->mhi_chan[chan].configured) { Same here. Thanks, Mani > mhi_chan = &mhi_cntrl->mhi_chan[chan]; > > if (likely(type == MHI_PKT_TYPE_TX_EVENT)) { > @@ -900,7 +950,15 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, > > mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring); > local_rp = ev_ring->rp; > - dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); > + > + ptr = 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"); > + return -EIO; > + } > + > + dev_rp = mhi_to_virtual(ev_ring, ptr); > count++; > } > read_lock_bh(&mhi_cntrl->pm_lock); > @@ -1365,6 +1423,7 @@ static void mhi_mark_stale_events(struct mhi_controller *mhi_cntrl, > struct mhi_ring *ev_ring; > struct device *dev = &mhi_cntrl->mhi_dev->dev; > unsigned long flags; > + dma_addr_t ptr; > > dev_dbg(dev, "Marking all events for chan: %d as stale\n", chan); > > @@ -1372,7 +1431,15 @@ static void mhi_mark_stale_events(struct mhi_controller *mhi_cntrl, > > /* mark all stale events related to channel as STALE event */ > spin_lock_irqsave(&mhi_event->lock, flags); > - dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); > + > + ptr = 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"); > + dev_rp = ev_ring->rp; > + } else { > + dev_rp = mhi_to_virtual(ev_ring, ptr); > + } > > local_rp = ev_ring->rp; > while (dev_rp != local_rp) { > -- > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project. >
On 2/24/2021 2:47 AM, Manivannan Sadhasivam wrote: > On Wed, Feb 17, 2021 at 09:20:22AM -0700, Jeffrey Hugo wrote: >> When parsing the structures in the shared memory, there are values which >> come from the remote device. For example, a transfer completion event >> will have a pointer to the tre in the relevant channel's transfer ring. >> Such values should be considered to be untrusted, and validated before >> use. If we blindly use such values, we may access invalid data or crash >> if the values are corrupted. >> >> If validation fails, drop the relevant event. >> >> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> >> --- >> >> v2: Fix subject >> >> drivers/bus/mhi/core/main.c | 81 +++++++++++++++++++++++++++++++++++++++++---- >> 1 file changed, 74 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c >> index c043574..1eb2fd3 100644 >> --- a/drivers/bus/mhi/core/main.c >> +++ b/drivers/bus/mhi/core/main.c >> @@ -242,6 +242,11 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, >> smp_wmb(); >> } >> >> +static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) >> +{ >> + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; >> +} >> + >> int mhi_destroy_device(struct device *dev, void *data) >> { >> struct mhi_device *mhi_dev; >> @@ -383,7 +388,16 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) >> struct mhi_event_ctxt *er_ctxt = >> &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; >> struct mhi_ring *ev_ring = &mhi_event->ring; >> - void *dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); >> + dma_addr_t ptr = er_ctxt->rp; >> + void *dev_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"); >> + return IRQ_HANDLED; >> + } >> + >> + dev_rp = mhi_to_virtual(ev_ring, ptr); >> >> /* Only proceed if event ring has pending events */ >> if (ev_ring->rp == dev_rp) >> @@ -536,6 +550,11 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, >> struct mhi_buf_info *buf_info; >> u16 xfer_len; >> >> + if (!is_valid_ring_ptr(tre_ring, ptr)) { >> + dev_err(&mhi_cntrl->mhi_dev->dev, >> + "Event element points outside of the tre ring\n"); >> + break; >> + } >> /* Get the TRB this event points to */ >> ev_tre = mhi_to_virtual(tre_ring, ptr); >> >> @@ -695,6 +714,12 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, >> struct mhi_chan *mhi_chan; >> u32 chan; >> >> + if (!is_valid_ring_ptr(mhi_ring, ptr)) { >> + dev_err(&mhi_cntrl->mhi_dev->dev, >> + "Event element points outside of the cmd ring\n"); >> + return; >> + } >> + >> cmd_pkt = mhi_to_virtual(mhi_ring, ptr); >> >> chan = MHI_TRE_GET_CMD_CHID(cmd_pkt); >> @@ -719,6 +744,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, >> struct device *dev = &mhi_cntrl->mhi_dev->dev; >> u32 chan; >> int count = 0; >> + dma_addr_t ptr = er_ctxt->rp; >> >> /* >> * This is a quick check to avoid unnecessary event processing >> @@ -728,7 +754,13 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, >> if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) >> return -EIO; >> >> - dev_rp = mhi_to_virtual(ev_ring, 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"); >> + return -EIO; >> + } >> + >> + dev_rp = mhi_to_virtual(ev_ring, ptr); >> local_rp = ev_ring->rp; >> >> while (dev_rp != local_rp) { >> @@ -834,6 +866,8 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, >> */ >> if (chan < mhi_cntrl->max_chan) { >> mhi_chan = &mhi_cntrl->mhi_chan[chan]; >> + if (!mhi_chan->configured) >> + break; > > This change is not part of this patch I believe. It is. The remote device specified an event on a channel. We already check to see that the specified channel value doesn't exceed the maximum number of channels, but we don't check to see that it is a valid channel within the range of channels. If its not a valid channel (say 0-5 and 7-10 are valid, max is 10, but the remote end specified 6), bad things could happen because we are implicitly trusting the value before fully checking its validity. This is still a sanity check of a value from the remote end.
On Wed, Feb 24, 2021 at 08:10:06AM -0700, Jeffrey Hugo wrote: > On 2/24/2021 2:47 AM, Manivannan Sadhasivam wrote: > > On Wed, Feb 17, 2021 at 09:20:22AM -0700, Jeffrey Hugo wrote: > > > When parsing the structures in the shared memory, there are values which > > > come from the remote device. For example, a transfer completion event > > > will have a pointer to the tre in the relevant channel's transfer ring. > > > Such values should be considered to be untrusted, and validated before > > > use. If we blindly use such values, we may access invalid data or crash > > > if the values are corrupted. > > > > > > If validation fails, drop the relevant event. > > > > > > Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> > > > --- > > > > > > v2: Fix subject > > > > > > drivers/bus/mhi/core/main.c | 81 +++++++++++++++++++++++++++++++++++++++++---- > > > 1 file changed, 74 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c > > > index c043574..1eb2fd3 100644 > > > --- a/drivers/bus/mhi/core/main.c > > > +++ b/drivers/bus/mhi/core/main.c > > > @@ -242,6 +242,11 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, > > > smp_wmb(); > > > } > > > +static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) > > > +{ > > > + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; > > > +} > > > + > > > int mhi_destroy_device(struct device *dev, void *data) > > > { > > > struct mhi_device *mhi_dev; > > > @@ -383,7 +388,16 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) > > > struct mhi_event_ctxt *er_ctxt = > > > &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; > > > struct mhi_ring *ev_ring = &mhi_event->ring; > > > - void *dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); > > > + dma_addr_t ptr = er_ctxt->rp; > > > + void *dev_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"); > > > + return IRQ_HANDLED; > > > + } > > > + > > > + dev_rp = mhi_to_virtual(ev_ring, ptr); > > > /* Only proceed if event ring has pending events */ > > > if (ev_ring->rp == dev_rp) > > > @@ -536,6 +550,11 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, > > > struct mhi_buf_info *buf_info; > > > u16 xfer_len; > > > + if (!is_valid_ring_ptr(tre_ring, ptr)) { > > > + dev_err(&mhi_cntrl->mhi_dev->dev, > > > + "Event element points outside of the tre ring\n"); > > > + break; > > > + } > > > /* Get the TRB this event points to */ > > > ev_tre = mhi_to_virtual(tre_ring, ptr); > > > @@ -695,6 +714,12 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, > > > struct mhi_chan *mhi_chan; > > > u32 chan; > > > + if (!is_valid_ring_ptr(mhi_ring, ptr)) { > > > + dev_err(&mhi_cntrl->mhi_dev->dev, > > > + "Event element points outside of the cmd ring\n"); > > > + return; > > > + } > > > + > > > cmd_pkt = mhi_to_virtual(mhi_ring, ptr); > > > chan = MHI_TRE_GET_CMD_CHID(cmd_pkt); > > > @@ -719,6 +744,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > > > struct device *dev = &mhi_cntrl->mhi_dev->dev; > > > u32 chan; > > > int count = 0; > > > + dma_addr_t ptr = er_ctxt->rp; > > > /* > > > * This is a quick check to avoid unnecessary event processing > > > @@ -728,7 +754,13 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > > > if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) > > > return -EIO; > > > - dev_rp = mhi_to_virtual(ev_ring, 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"); > > > + return -EIO; > > > + } > > > + > > > + dev_rp = mhi_to_virtual(ev_ring, ptr); > > > local_rp = ev_ring->rp; > > > while (dev_rp != local_rp) { > > > @@ -834,6 +866,8 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > > > */ > > > if (chan < mhi_cntrl->max_chan) { > > > mhi_chan = &mhi_cntrl->mhi_chan[chan]; > > > + if (!mhi_chan->configured) > > > + break; > > > > This change is not part of this patch I believe. > > It is. The remote device specified an event on a channel. We already check > to see that the specified channel value doesn't exceed the maximum number of > channels, but we don't check to see that it is a valid channel within the > range of channels. If its not a valid channel (say 0-5 and 7-10 are valid, > max is 10, but the remote end specified 6), bad things could happen because > we are implicitly trusting the value before fully checking its validity. > > This is still a sanity check of a value from the remote end. > Okay. Please mention it in the commit message. Currently it mentions only the tre pointer. Thanks, Mani > -- > Jeffrey Hugo > Qualcomm Technologies, Inc. is a member of the > Code Aurora Forum, a Linux Foundation Collaborative Project.
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c index c043574..1eb2fd3 100644 --- a/drivers/bus/mhi/core/main.c +++ b/drivers/bus/mhi/core/main.c @@ -242,6 +242,11 @@ static void mhi_del_ring_element(struct mhi_controller *mhi_cntrl, smp_wmb(); } +static bool is_valid_ring_ptr(struct mhi_ring *ring, dma_addr_t addr) +{ + return addr >= ring->iommu_base && addr < ring->iommu_base + ring->len; +} + int mhi_destroy_device(struct device *dev, void *data) { struct mhi_device *mhi_dev; @@ -383,7 +388,16 @@ irqreturn_t mhi_irq_handler(int irq_number, void *dev) struct mhi_event_ctxt *er_ctxt = &mhi_cntrl->mhi_ctxt->er_ctxt[mhi_event->er_index]; struct mhi_ring *ev_ring = &mhi_event->ring; - void *dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); + dma_addr_t ptr = er_ctxt->rp; + void *dev_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"); + return IRQ_HANDLED; + } + + dev_rp = mhi_to_virtual(ev_ring, ptr); /* Only proceed if event ring has pending events */ if (ev_ring->rp == dev_rp) @@ -536,6 +550,11 @@ static int parse_xfer_event(struct mhi_controller *mhi_cntrl, struct mhi_buf_info *buf_info; u16 xfer_len; + if (!is_valid_ring_ptr(tre_ring, ptr)) { + dev_err(&mhi_cntrl->mhi_dev->dev, + "Event element points outside of the tre ring\n"); + break; + } /* Get the TRB this event points to */ ev_tre = mhi_to_virtual(tre_ring, ptr); @@ -695,6 +714,12 @@ static void mhi_process_cmd_completion(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan; u32 chan; + if (!is_valid_ring_ptr(mhi_ring, ptr)) { + dev_err(&mhi_cntrl->mhi_dev->dev, + "Event element points outside of the cmd ring\n"); + return; + } + cmd_pkt = mhi_to_virtual(mhi_ring, ptr); chan = MHI_TRE_GET_CMD_CHID(cmd_pkt); @@ -719,6 +744,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, struct device *dev = &mhi_cntrl->mhi_dev->dev; u32 chan; int count = 0; + dma_addr_t ptr = er_ctxt->rp; /* * This is a quick check to avoid unnecessary event processing @@ -728,7 +754,13 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) return -EIO; - dev_rp = mhi_to_virtual(ev_ring, 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"); + return -EIO; + } + + dev_rp = mhi_to_virtual(ev_ring, ptr); local_rp = ev_ring->rp; while (dev_rp != local_rp) { @@ -834,6 +866,8 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, */ if (chan < mhi_cntrl->max_chan) { mhi_chan = &mhi_cntrl->mhi_chan[chan]; + if (!mhi_chan->configured) + break; parse_xfer_event(mhi_cntrl, local_rp, mhi_chan); event_quota--; } @@ -845,7 +879,15 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring); local_rp = ev_ring->rp; - dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); + + ptr = 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"); + return -EIO; + } + + dev_rp = mhi_to_virtual(ev_ring, ptr); count++; } @@ -868,11 +910,18 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, int count = 0; u32 chan; struct mhi_chan *mhi_chan; + dma_addr_t ptr = er_ctxt->rp; if (unlikely(MHI_EVENT_ACCESS_INVALID(mhi_cntrl->pm_state))) return -EIO; - dev_rp = mhi_to_virtual(ev_ring, 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"); + return -EIO; + } + + dev_rp = mhi_to_virtual(ev_ring, ptr); local_rp = ev_ring->rp; while (dev_rp != local_rp && event_quota > 0) { @@ -886,7 +935,8 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, * Only process the event ring elements whose channel * ID is within the maximum supported range. */ - if (chan < mhi_cntrl->max_chan) { + if (chan < mhi_cntrl->max_chan && + mhi_cntrl->mhi_chan[chan].configured) { mhi_chan = &mhi_cntrl->mhi_chan[chan]; if (likely(type == MHI_PKT_TYPE_TX_EVENT)) { @@ -900,7 +950,15 @@ int mhi_process_data_event_ring(struct mhi_controller *mhi_cntrl, mhi_recycle_ev_ring_element(mhi_cntrl, ev_ring); local_rp = ev_ring->rp; - dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); + + ptr = 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"); + return -EIO; + } + + dev_rp = mhi_to_virtual(ev_ring, ptr); count++; } read_lock_bh(&mhi_cntrl->pm_lock); @@ -1365,6 +1423,7 @@ static void mhi_mark_stale_events(struct mhi_controller *mhi_cntrl, struct mhi_ring *ev_ring; struct device *dev = &mhi_cntrl->mhi_dev->dev; unsigned long flags; + dma_addr_t ptr; dev_dbg(dev, "Marking all events for chan: %d as stale\n", chan); @@ -1372,7 +1431,15 @@ static void mhi_mark_stale_events(struct mhi_controller *mhi_cntrl, /* mark all stale events related to channel as STALE event */ spin_lock_irqsave(&mhi_event->lock, flags); - dev_rp = mhi_to_virtual(ev_ring, er_ctxt->rp); + + ptr = 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"); + dev_rp = ev_ring->rp; + } else { + dev_rp = mhi_to_virtual(ev_ring, ptr); + } local_rp = ev_ring->rp; while (dev_rp != local_rp) {
When parsing the structures in the shared memory, there are values which come from the remote device. For example, a transfer completion event will have a pointer to the tre in the relevant channel's transfer ring. Such values should be considered to be untrusted, and validated before use. If we blindly use such values, we may access invalid data or crash if the values are corrupted. If validation fails, drop the relevant event. Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org> --- v2: Fix subject drivers/bus/mhi/core/main.c | 81 +++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 74 insertions(+), 7 deletions(-)