* [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() @ 2019-06-08 9:25 Dan Carpenter 2019-10-07 12:18 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2019-06-08 9:25 UTC (permalink / raw) To: kernel-janitors 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 related [flat|nested] 9+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() 2019-06-08 9:25 [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() Dan Carpenter @ 2019-10-07 12:18 ` Dan Carpenter 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() @ 2019-10-07 12:18 ` Dan Carpenter 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() 2019-10-07 12:18 ` Dan Carpenter @ 2019-10-24 16:37 ` Jason Gunthorpe -1 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() @ 2019-10-24 16:37 ` Jason Gunthorpe 0 siblings, 0 replies; 9+ 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] 9+ 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 -1 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() @ 2019-10-24 18:20 ` Dan Carpenter 0 siblings, 0 replies; 9+ 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] 9+ 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 -1 siblings, 0 replies; 9+ 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] 9+ messages in thread
* Re: [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() @ 2019-10-28 14:23 ` Jason Gunthorpe 0 siblings, 0 replies; 9+ 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] 9+ messages in thread
end of thread, other threads:[~2019-10-28 14:23 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-06-08 9:25 [PATCH] RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size() Dan Carpenter 2019-10-07 12:18 ` Dan Carpenter 2019-10-07 12:18 ` Dan Carpenter 2019-10-24 16:37 ` Jason Gunthorpe 2019-10-24 16:37 ` Jason Gunthorpe 2019-10-24 18:20 ` Dan Carpenter 2019-10-24 18:20 ` Dan Carpenter 2019-10-28 14:23 ` Jason Gunthorpe 2019-10-28 14:23 ` Jason Gunthorpe
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.