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

2020-05-05 Thread bbhatt 

On 2020-05-05 08:57, Jeffrey Hugo wrote:

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 
---
  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   0x
+#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?


I was under the impression that prandom_u32_max will return a value 
between 0 to
whatever is supplied (in your example 7) and not 6. I noticed the 
description has

the round bracket to indicate that it is excluded.

If there is no need to do a bmsk - 1 then what you said makes sense.

Main thing is to not go above the mask and to get a random non-zero 
value which

fits within the mask.

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

2020-05-05 Thread Jeffrey Hugo 

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 
---
  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   0x
+#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.

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

2020-05-04 Thread Bhaumik Bhatt 
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 
---
 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_DB127
 #define MHI_MAX_MTU0x
+#define MHI_RANDOM_U32_NONZERO(bmsk)   ((prandom_u32_max(U32_MAX - 1) & \
+(bmsk)) + 1)
 
 enum mhi_er_type {
MHI_ER_TYPE_INVALID = 0x0,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project