[PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Dan Carpenter]

The "ucmd->log_sq_bb_count" variable is a user controlled variable in
the 0-255 range.  If we shift more than then number of bits in an int
then it's undefined behavior (it shift wraps).  It turns out this
doesn't cause any real issues at runtime, but it's good to check anyway.

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
 drivers/infiniband/hw/hns/hns_roce_qp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index 8db2817a249e..006b3e7f4ed5 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -342,7 +342,8 @@ static int hns_roce_set_user_sq_size(struct hns_roce_dev *hr_dev,
 	u32 max_cnt;
 
 	/* Sanity check SQ size before proceeding */
-	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
+	if (ucmd->log_sq_bb_count > 31 ||
+	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
 	     ucmd->log_sq_stride > max_sq_stride ||
 	     ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
 		dev_err(hr_dev->dev, "check SQ size error!\n");
-- 
2.20.1

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Dan Carpenter]

This one still needs to be applied.

regards,
dan carpenter

On Sat, Jun 08, 2019 at 12:25:14PM +0300, Dan Carpenter wrote:
> The "ucmd->log_sq_bb_count" variable is a user controlled variable in
> the 0-255 range.  If we shift more than then number of bits in an int
> then it's undefined behavior (it shift wraps).  It turns out this
> doesn't cause any real issues at runtime, but it's good to check anyway.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_qp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
> index 8db2817a249e..006b3e7f4ed5 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_qp.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
> @@ -342,7 +342,8 @@ static int hns_roce_set_user_sq_size(struct hns_roce_dev *hr_dev,
>  	u32 max_cnt;
>  
>  	/* Sanity check SQ size before proceeding */
> -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> +	if (ucmd->log_sq_bb_count > 31 ||
> +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
>  	     ucmd->log_sq_stride > max_sq_stride ||
>  	     ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
>  		dev_err(hr_dev->dev, "check SQ size error!\n");
> -- 
> 2.20.1

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Dan Carpenter]

This one still needs to be applied.

regards,
dan carpenter

On Sat, Jun 08, 2019 at 12:25:14PM +0300, Dan Carpenter wrote:
> The "ucmd->log_sq_bb_count" variable is a user controlled variable in
> the 0-255 range.  If we shift more than then number of bits in an int
> then it's undefined behavior (it shift wraps).  It turns out this
> doesn't cause any real issues at runtime, but it's good to check anyway.
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>  drivers/infiniband/hw/hns/hns_roce_qp.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
> index 8db2817a249e..006b3e7f4ed5 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_qp.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
> @@ -342,7 +342,8 @@ static int hns_roce_set_user_sq_size(struct hns_roce_dev *hr_dev,
>  	u32 max_cnt;
>  
>  	/* Sanity check SQ size before proceeding */
> -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> +	if (ucmd->log_sq_bb_count > 31 ||
> +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
>  	     ucmd->log_sq_stride > max_sq_stride ||
>  	     ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
>  		dev_err(hr_dev->dev, "check SQ size error!\n");
> -- 
> 2.20.1

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Jason Gunthorpe]

On Mon, Oct 07, 2019 at 03:18:22PM +0300, Dan Carpenter wrote:
> This one still needs to be applied.
> 
> regards,
> dan carpenter

Weird, it is marked changes requested in patchworks. An email must
have been lost??

I think I probably wanted to say that:

> >  	/* Sanity check SQ size before proceeding */
> > -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> > +	if (ucmd->log_sq_bb_count > 31 ||
> > +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes
> > ||

Overall should probably be coded using check_shl_overflow(), as this
later shift:

	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;

Is storing it into an int and hardwring '31' because it magically
matches that lower shift is pretty fragile.

More like this?

diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index bec48f2593f405..6aa27d6ea3a600 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -332,9 +332,8 @@ static int check_sq_size_with_integrity(struct hns_roce_dev *hr_dev,
 	u8 max_sq_stride = ilog2(roundup_sq_stride);
 
 	/* Sanity check SQ size before proceeding */
-	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
-	     ucmd->log_sq_stride > max_sq_stride ||
-	     ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
+	if (ucmd->log_sq_stride > max_sq_stride ||
+	    ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
 		ibdev_err(&hr_dev->ib_dev, "check SQ size error!\n");
 		return -EINVAL;
 	}
@@ -358,13 +357,16 @@ static int hns_roce_set_user_sq_size(struct hns_roce_dev *hr_dev,
 	u32 max_cnt;
 	int ret;
 
+	if (check_shl_overflow(1, ucmd->log_sq_bb_count, &hr_qp->sq.wqe_cnt) ||
+	    hr_qp->sq.wqe_cnt > hr_dev->caps.max_wqes)
+		return -EINVAL;
+
 	ret = check_sq_size_with_integrity(hr_dev, cap, ucmd);
 	if (ret) {
 		ibdev_err(&hr_dev->ib_dev, "Sanity check sq size failed\n");
 		return ret;
 	}
 
-	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;
 	hr_qp->sq.wqe_shift = ucmd->log_sq_stride;
 
 	max_cnt = max(1U, cap->max_send_sge);

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Jason Gunthorpe]

On Mon, Oct 07, 2019 at 03:18:22PM +0300, Dan Carpenter wrote:
> This one still needs to be applied.
> 
> regards,
> dan carpenter

Weird, it is marked changes requested in patchworks. An email must
have been lost??

I think I probably wanted to say that:

> >  	/* Sanity check SQ size before proceeding */
> > -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> > +	if (ucmd->log_sq_bb_count > 31 ||
> > +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes
> > ||

Overall should probably be coded using check_shl_overflow(), as this
later shift:

	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;

Is storing it into an int and hardwring '31' because it magically
matches that lower shift is pretty fragile.

More like this?

diff --git a/drivers/infiniband/hw/hns/hns_roce_qp.c b/drivers/infiniband/hw/hns/hns_roce_qp.c
index bec48f2593f405..6aa27d6ea3a600 100644
--- a/drivers/infiniband/hw/hns/hns_roce_qp.c
+++ b/drivers/infiniband/hw/hns/hns_roce_qp.c
@@ -332,9 +332,8 @@ static int check_sq_size_with_integrity(struct hns_roce_dev *hr_dev,
 	u8 max_sq_stride = ilog2(roundup_sq_stride);
 
 	/* Sanity check SQ size before proceeding */
-	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
-	     ucmd->log_sq_stride > max_sq_stride ||
-	     ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
+	if (ucmd->log_sq_stride > max_sq_stride ||
+	    ucmd->log_sq_stride < HNS_ROCE_IB_MIN_SQ_STRIDE) {
 		ibdev_err(&hr_dev->ib_dev, "check SQ size error!\n");
 		return -EINVAL;
 	}
@@ -358,13 +357,16 @@ static int hns_roce_set_user_sq_size(struct hns_roce_dev *hr_dev,
 	u32 max_cnt;
 	int ret;
 
+	if (check_shl_overflow(1, ucmd->log_sq_bb_count, &hr_qp->sq.wqe_cnt) ||
+	    hr_qp->sq.wqe_cnt > hr_dev->caps.max_wqes)
+		return -EINVAL;
+
 	ret = check_sq_size_with_integrity(hr_dev, cap, ucmd);
 	if (ret) {
 		ibdev_err(&hr_dev->ib_dev, "Sanity check sq size failed\n");
 		return ret;
 	}
 
-	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;
 	hr_qp->sq.wqe_shift = ucmd->log_sq_stride;
 
 	max_cnt = max(1U, cap->max_send_sge);

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Dan Carpenter]

On Thu, Oct 24, 2019 at 01:37:49PM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 07, 2019 at 03:18:22PM +0300, Dan Carpenter wrote:
> > This one still needs to be applied.
> > 
> > regards,
> > dan carpenter
> 
> Weird, it is marked changes requested in patchworks. An email must
> have been lost??
>

Maybe you replied to a different thread?

> I think I probably wanted to say that:
> 
> > >  	/* Sanity check SQ size before proceeding */
> > > -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> > > +	if (ucmd->log_sq_bb_count > 31 ||
> > > +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes
> > > ||
> 
> Overall should probably be coded using check_shl_overflow(), as this
> later shift:
> 
> 	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;
> 
> Is storing it into an int and hardwring '31' because it magically
> matches that lower shift is pretty fragile.
> 
> More like this?
> 

Yeah.  I like your patch.  I'd feel silly claiming authorship though.
I'm willing to, because it's nice, but probably you should just give me
Reported-by credit instead.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Dan Carpenter]

On Thu, Oct 24, 2019 at 01:37:49PM -0300, Jason Gunthorpe wrote:
> On Mon, Oct 07, 2019 at 03:18:22PM +0300, Dan Carpenter wrote:
> > This one still needs to be applied.
> > 
> > regards,
> > dan carpenter
> 
> Weird, it is marked changes requested in patchworks. An email must
> have been lost??
>

Maybe you replied to a different thread?

> I think I probably wanted to say that:
> 
> > >  	/* Sanity check SQ size before proceeding */
> > > -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> > > +	if (ucmd->log_sq_bb_count > 31 ||
> > > +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes
> > > ||
> 
> Overall should probably be coded using check_shl_overflow(), as this
> later shift:
> 
> 	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;
> 
> Is storing it into an int and hardwring '31' because it magically
> matches that lower shift is pretty fragile.
> 
> More like this?
> 

Yeah.  I like your patch.  I'd feel silly claiming authorship though.
I'm willing to, because it's nice, but probably you should just give me
Reported-by credit instead.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Jason Gunthorpe]

On Thu, Oct 24, 2019 at 09:20:39PM +0300, Dan Carpenter wrote:
> On Thu, Oct 24, 2019 at 01:37:49PM -0300, Jason Gunthorpe wrote:
> > On Mon, Oct 07, 2019 at 03:18:22PM +0300, Dan Carpenter wrote:
> > > This one still needs to be applied.
> > > 
> > > regards,
> > > dan carpenter
> > 
> > Weird, it is marked changes requested in patchworks. An email must
> > have been lost??
> >
> 
> Maybe you replied to a different thread?
> 
> > I think I probably wanted to say that:
> > 
> > > >  	/* Sanity check SQ size before proceeding */
> > > > -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> > > > +	if (ucmd->log_sq_bb_count > 31 ||
> > > > +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes
> > > > ||
> > 
> > Overall should probably be coded using check_shl_overflow(), as this
> > later shift:
> > 
> > 	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;
> > 
> > Is storing it into an int and hardwring '31' because it magically
> > matches that lower shift is pretty fragile.
> > 
> > More like this?
> > 
> 
> Yeah.  I like your patch.  I'd feel silly claiming authorship though.
> I'm willing to, because it's nice, but probably you should just give me
> Reported-by credit instead.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

Okay applied to for-next

Jason

Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()

[Jason Gunthorpe]

On Thu, Oct 24, 2019 at 09:20:39PM +0300, Dan Carpenter wrote:
> On Thu, Oct 24, 2019 at 01:37:49PM -0300, Jason Gunthorpe wrote:
> > On Mon, Oct 07, 2019 at 03:18:22PM +0300, Dan Carpenter wrote:
> > > This one still needs to be applied.
> > > 
> > > regards,
> > > dan carpenter
> > 
> > Weird, it is marked changes requested in patchworks. An email must
> > have been lost??
> >
> 
> Maybe you replied to a different thread?
> 
> > I think I probably wanted to say that:
> > 
> > > >  	/* Sanity check SQ size before proceeding */
> > > > -	if ((u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes ||
> > > > +	if (ucmd->log_sq_bb_count > 31 ||
> > > > +	    (u32)(1 << ucmd->log_sq_bb_count) > hr_dev->caps.max_wqes
> > > > ||
> > 
> > Overall should probably be coded using check_shl_overflow(), as this
> > later shift:
> > 
> > 	hr_qp->sq.wqe_cnt = 1 << ucmd->log_sq_bb_count;
> > 
> > Is storing it into an int and hardwring '31' because it magically
> > matches that lower shift is pretty fragile.
> > 
> > More like this?
> > 
> 
> Yeah.  I like your patch.  I'd feel silly claiming authorship though.
> I'm willing to, because it's nice, but probably you should just give me
> Reported-by credit instead.
> 
> Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

Okay applied to for-next

Jason