Re: [PATCH v5 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values are used

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

 



On 5/4/2020 8:44 PM, Bhaumik Bhatt wrote:
While writing any sequence or session identifiers, it is possible that
the host could write a zero value, whereas only non-zero values should
be supported writes to those registers. Ensure that the host does not
write a non-zero value for them and also log them in debug messages.

Signed-off-by: Bhaumik Bhatt <bbhatt@xxxxxxxxxxxxxx>
---
  drivers/bus/mhi/core/boot.c     | 15 +++++++--------
  drivers/bus/mhi/core/internal.h |  2 ++
  2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index e5fcde1..7b9b561 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -43,10 +43,7 @@ void mhi_rddm_prepare(struct mhi_controller *mhi_cntrl,
  		      lower_32_bits(mhi_buf->dma_addr));
mhi_write_reg(mhi_cntrl, base, BHIE_RXVECSIZE_OFFS, mhi_buf->len);
-	sequence_id = prandom_u32() & BHIE_RXVECSTATUS_SEQNUM_BMSK;
-
-	if (unlikely(!sequence_id))
-		sequence_id = 1;
+	sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_RXVECSTATUS_SEQNUM_BMSK);
mhi_write_reg_field(mhi_cntrl, base, BHIE_RXVECDB_OFFS,
  			    BHIE_RXVECDB_SEQNUM_BMSK, BHIE_RXVECDB_SEQNUM_SHFT,
@@ -189,7 +186,9 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl,
  		return -EIO;
  	}
- dev_dbg(dev, "Starting AMSS download via BHIe\n");
+	sequence_id = MHI_RANDOM_U32_NONZERO(BHIE_TXVECSTATUS_SEQNUM_BMSK);
+	dev_dbg(dev, "Starting AMSS download via BHIe. Sequence ID:%u\n",
+		sequence_id);
  	mhi_write_reg(mhi_cntrl, base, BHIE_TXVECADDR_HIGH_OFFS,
  		      upper_32_bits(mhi_buf->dma_addr));
@@ -198,7 +197,6 @@ static int mhi_fw_load_amss(struct mhi_controller *mhi_cntrl, mhi_write_reg(mhi_cntrl, base, BHIE_TXVECSIZE_OFFS, mhi_buf->len); - sequence_id = prandom_u32() & BHIE_TXVECSTATUS_SEQNUM_BMSK;
  	mhi_write_reg_field(mhi_cntrl, base, BHIE_TXVECDB_OFFS,
  			    BHIE_TXVECDB_SEQNUM_BMSK, BHIE_TXVECDB_SEQNUM_SHFT,
  			    sequence_id);
@@ -246,14 +244,15 @@ static int mhi_fw_load_sbl(struct mhi_controller *mhi_cntrl,
  		goto invalid_pm_state;
  	}
- dev_dbg(dev, "Starting SBL download via BHI\n");
+	session_id = MHI_RANDOM_U32_NONZERO(BHI_TXDB_SEQNUM_BMSK);
+	dev_dbg(dev, "Starting SBL download via BHI. Session ID:%u\n",
+		session_id);
  	mhi_write_reg(mhi_cntrl, base, BHI_STATUS, 0);
  	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_HIGH,
  		      upper_32_bits(dma_addr));
  	mhi_write_reg(mhi_cntrl, base, BHI_IMGADDR_LOW,
  		      lower_32_bits(dma_addr));
  	mhi_write_reg(mhi_cntrl, base, BHI_IMGSIZE, size);
-	session_id = prandom_u32() & BHI_TXDB_SEQNUM_BMSK;
  	mhi_write_reg(mhi_cntrl, base, BHI_IMGTXDB, session_id);
  	read_unlock_bh(pm_lock);
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 0965ca3..3205a92 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -452,6 +452,8 @@ enum mhi_pm_state {
  #define PRIMARY_CMD_RING		0
  #define MHI_DEV_WAKE_DB			127
  #define MHI_MAX_MTU			0xffff
+#define MHI_RANDOM_U32_NONZERO(bmsk)	((prandom_u32_max(U32_MAX - 1) & \
+					 (bmsk)) + 1)

I still think this is broken.  I'm sorry for the back and forth.

So, again if prandom_u32_max returns 0xFF, and bmsk is 0xF, we get 0xF by the & operation, then we add 1, which makes the result 0x10, which is outside of the range of bmsk, and is basically 0, assuming the register doesn't accept values outside of the lower 4 bits.

I think the solution should be:
prandom_u32_max(bmsk) + 1

If we treat bmsk like a ordinary value (say 7), then prandom_u32_max will return a value from 0-6. Then by adding 1, we shift that range to 1-7, which I think is exactly what we want.

Now, this assumes that bmsk is a contiguous mask of bits from bit 0 to N. IE 0xFF and 0x4F are valid, but 0xFB is not. Do you think that is a valid assumption?

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