RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()
Subsystems (3)
PATCH 1/1 RDMA/hns: prevent undefined behavior in hns_roce_set_user_sq_size()
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.
--- drivers/infiniband/hw/hns/hns_roce_qp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/infiniband/hw/hns/hns_roce_qp.c +2 -1
@@ -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" );
Signature · 1 line
2.20.1
This one still needs to be applied.
regards,
dan carpenter
Quoted 26 lines · 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 <[email protected]>--- drivers/infiniband/hw/hns/hns_roce_qp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/infiniband/hw/hns/hns_roce_qp.c +2
-1@@ -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" );Signature · 1 line
2.20.1
Quoted 4 lines · Dan Carpenter
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:
Quoted 5 lines
Quoted 5 lines
(quoted diff) +2
-1/* 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?
drivers/infiniband/hw/hns/hns_roce_qp.c +6 -4
@@ -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);
Dan Carpenter
Reviewed-byQuoted 9 lines · Jason Gunthorpe
Quoted 4 lines · Dan Carpenter
This one still needs to be applied.
regards,
dan carpenterWeird, it is marked changes requested in patchworks. An email must have been lost??
Maybe you replied to a different thread?
Quoted 11 lines
I think I probably wanted to say that:
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/hw/hns/hns_roce_qp.c +2
-1/* 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:
Quoted 7 lines
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.
regards,
dan carpenter
Quoted 29 lines · Dan Carpenter
Quoted 9 lines · Jason Gunthorpe
Quoted 4 lines · Dan Carpenter
This one still needs to be applied.
regards,
dan carpenterWeird, it is marked changes requested in patchworks. An email must have been lost??
Maybe you replied to a different thread?
Quoted 16 lines
I think I probably wanted to say that:
Quoted 5 lines
Quoted 5 lines
drivers/infiniband/hw/hns/hns_roce_qp.c +2
-1/* 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.
Quoted 8 lines
Quoted 2 lines
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 <[email protected]>
Okay applied to for-next
Jason