* 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).