* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() [not found] <20190608092514.GC28890@mwanda> @ 2019-10-07 12:18 ` Dan Carpenter 2019-10-24 16:37 ` Jason Gunthorpe 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2019-10-07 12:18 UTC (permalink / raw) To: Lijun Ou Cc: Wei Hu(Xavier), Doug Ledford, Jason Gunthorpe, linux-rdma, kernel-janitors 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() 2019-10-07 12:18 ` [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() Dan Carpenter @ 2019-10-24 16:37 ` Jason Gunthorpe 2019-10-24 18:20 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Jason Gunthorpe @ 2019-10-24 16:37 UTC (permalink / raw) To: Dan Carpenter Cc: Lijun Ou, Wei Hu(Xavier), Doug Ledford, linux-rdma, kernel-janitors 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); ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() 2019-10-24 16:37 ` Jason Gunthorpe @ 2019-10-24 18:20 ` Dan Carpenter 2019-10-28 14:23 ` Jason Gunthorpe 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2019-10-24 18:20 UTC (permalink / raw) To: Jason Gunthorpe Cc: Lijun Ou, Wei Hu(Xavier), Doug Ledford, linux-rdma, kernel-janitors 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() 2019-10-24 18:20 ` Dan Carpenter @ 2019-10-28 14:23 ` Jason Gunthorpe 0 siblings, 0 replies; 4+ messages in thread From: Jason Gunthorpe @ 2019-10-28 14:23 UTC (permalink / raw) To: Dan Carpenter Cc: Lijun Ou, Wei Hu(Xavier), Doug Ledford, linux-rdma, kernel-janitors 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-28 14:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20190608092514.GC28890@mwanda>
2019-10-07 12:18 ` [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() Dan Carpenter
2019-10-24 16:37 ` Jason Gunthorpe
2019-10-24 18:20 ` Dan Carpenter
2019-10-28 14:23 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).