| Message ID | 20190608092514.GC28890@mwanda (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() | expand |
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
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);
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
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
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");
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(-)