Re: [PATCH v2] bus: mhi: core: Sanity check values from remote device before use

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxx>
---

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.

--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux