On Sun, Aug 12, 2018 at 02:42:58PM +0300, Yishai Hadas wrote:
> On 8/10/2018 12:06 AM, Jason Gunthorpe wrote:
> > mlx5_ib_create_qp_resp was never initialized and only the first 4 bytes
> > were written. Static checkers missed this because the struct was
> > un-necessarily created in a different function, so consolidate that too.
> >
> > Fixes: 41d902cb7c32 ("RDMA/mlx5: Fix definition of mlx5_ib_create_qp_resp")
> > Cc: <stable@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
> > drivers/infiniband/hw/mlx5/qp.c | 14 ++++++--------
> > 1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
> > index 6efd770797d121..5839ac0083fa07 100644
> > +++ b/drivers/infiniband/hw/mlx5/qp.c
> > @@ -772,14 +772,13 @@ static int adjust_bfregn(struct mlx5_ib_dev *dev,
> > static int create_user_qp(struct mlx5_ib_dev *dev, struct ib_pd *pd,
> > struct mlx5_ib_qp *qp, struct ib_udata *udata,
> > - struct ib_qp_init_attr *attr,
> > - u32 **in,
> > - struct mlx5_ib_create_qp_resp *resp, int *inlen,
> > + struct ib_qp_init_attr *attr, u32 **in, int *inlen,
> > struct mlx5_ib_qp_base *base)
> > {
> > struct mlx5_ib_ucontext *context;
> > struct mlx5_ib_create_qp ucmd;
> > struct mlx5_ib_ubuffer *ubuffer = &base->ubuffer;
> > + struct mlx5_ib_create_qp_resp resp = {};
> > int page_shift = 0;
> > int uar_index = 0;
> > int npages;
> > @@ -861,9 +860,9 @@ static int create_user_qp(struct mlx5_ib_dev *dev, struct ib_pd *pd,
> > MLX5_SET(qpc, qpc, uar_page, uar_index);
> > if (bfregn != MLX5_IB_INVALID_BFREG)
> > - resp->bfreg_index = adjust_bfregn(dev, &context->bfregi, bfregn);
> > + resp.bfreg_index = adjust_bfregn(dev, &context->bfregi, bfregn);
> > else
> > - resp->bfreg_index = MLX5_IB_INVALID_BFREG;
> > + resp.bfreg_index = MLX5_IB_INVALID_BFREG;
> > qp->bfregn = bfregn;
> > err = mlx5_ib_db_map_user(context, ucmd.db_addr, &qp->db);
> > @@ -872,7 +871,7 @@ static int create_user_qp(struct mlx5_ib_dev *dev, struct ib_pd *pd,
> > goto err_free;
> > }
> > - err = ib_copy_to_udata(udata, resp, min(udata->outlen, sizeof(*resp)));
> > + err = ib_copy_to_udata(udata, &resp, min(udata->outlen, sizeof(resp)));
> > if (err) {
> > mlx5_ib_dbg(dev, "copy failed\n");
> > goto err_unmap;
> > @@ -1607,7 +1606,6 @@ static int create_qp_common(struct mlx5_ib_dev *dev, struct ib_pd *pd,
> > struct mlx5_ib_resources *devr = &dev->devr;
> > int inlen = MLX5_ST_SZ_BYTES(create_qp_in);
> > struct mlx5_core_dev *mdev = dev->mdev;
> > - struct mlx5_ib_create_qp_resp resp;
>
> I would prefer leave it here and set the fix (i.e. = {}) instead of the
> local 'resp' that this patch introduced as part of create_user_qp().
> Down the road we plan some extensions to the response that will use this
> field (e.g. as part of create_raw_packet_qp() below for TIR/TIS).
Okay.. The revised patch is much simpler. I've queued it to the WIP
for-next branch.
commit f0b7c809ae1405cf9beb71541015141537f50193 (HEAD -> k.o/for-next)
Author: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
Date: Tue Aug 14 15:33:52 2018 -0600
IB/mlx5: Fix leaking stack memory to userspace
mlx5_ib_create_qp_resp was never initialized and only the first 4 bytes
were written.
Fixes: 41d902cb7c32 ("RDMA/mlx5: Fix definition of mlx5_ib_create_qp_resp")
Cc: <stable@xxxxxxxxxxxxxxx>
Acked-by: Leon Romanovsky <leonro@xxxxxxxxxxxx>
Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 351c2efceb355c..6cba2a02d11bad 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -1607,7 +1607,7 @@ static int create_qp_common(struct mlx5_ib_dev *dev, struct ib_pd *pd,
struct mlx5_ib_resources *devr = &dev->devr;
int inlen = MLX5_ST_SZ_BYTES(create_qp_in);
struct mlx5_core_dev *mdev = dev->mdev;
- struct mlx5_ib_create_qp_resp resp;
+ struct mlx5_ib_create_qp_resp resp = {};
struct mlx5_ib_cq *send_cq;
struct mlx5_ib_cq *recv_cq;
unsigned long flags;
Jason
