lkml.org 
[lkml]   [2020]   [May]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v5 8/8] bus: mhi: core: Ensure non-zero session or sequence ID values are used
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 <bbhatt@codeaurora.org>
>> ---
>> 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?

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.

\
 
 \ /
  Last update: 2020-05-05 20:15    [W:0.051 / U:0.804 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site