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

[bbhatt@xxxxxxxxxxxxxx]

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

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.