| Message ID | 20180809210655.GA3574@ziepe.ca (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | IB/mlx5: Fix leaking stack memory to userspace | expand |
On Thu, Aug 09, 2018 at 03:06:55PM -0600, 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@vger.kernel.org> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- Except that mentioned "Fixes" is not related and patch subject is misleading. Userspace simply see garbage memory which belongs to mlx5_ib_create_qp_resp and not to "stack memory". Better to write "Clear create QP response returned to userspace" I'm fine with that. Acked-by: Leon Romanovsky <leonro@mellanox.com>
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@vger.kernel.org> > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > --- > 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 > --- a/drivers/infiniband/hw/mlx5/qp.c > +++ 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). > struct mlx5_ib_cq *send_cq; > struct mlx5_ib_cq *recv_cq; > unsigned long flags; > @@ -1763,7 +1761,7 @@ static int create_qp_common(struct mlx5_ib_dev *dev, struct ib_pd *pd, > return -EINVAL; > } > err = create_user_qp(dev, pd, qp, udata, init_attr, &in, > - &resp, &inlen, base); > + &inlen, base); > if (err) > mlx5_ib_dbg(dev, "err %d\n", err); > } else { >
On Sat, Aug 11, 2018 at 10:43:42AM +0300, Leon Romanovsky wrote: > On Thu, Aug 09, 2018 at 03:06:55PM -0600, 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@vger.kernel.org> > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > --- > > Except that mentioned "Fixes" is not related and patch subject is > misleading. The patch in fixes created the bug by extending the structure and not intializing the new fields. > Userspace simply see garbage memory which belongs to > mlx5_ib_create_qp_resp and not to "stack memory". mlx5_ib_create_qp_resp is allocated on the stack, so it is properly called kernel "stack memory" Jason
On Sun, Aug 12, 2018 at 11:11:17AM -0600, Jason Gunthorpe wrote: > On Sat, Aug 11, 2018 at 10:43:42AM +0300, Leon Romanovsky wrote: > > On Thu, Aug 09, 2018 at 03:06:55PM -0600, 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@vger.kernel.org> > > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > > --- > > > > Except that mentioned "Fixes" is not related and patch subject is > > misleading. > > The patch in fixes created the bug by extending the structure and > not intializing the new fields. New fields == "reserved". No one should care about the value in that field. > > > Userspace simply see garbage memory which belongs to > > mlx5_ib_create_qp_resp and not to "stack memory". > > mlx5_ib_create_qp_resp is allocated on the stack, so it is properly > called kernel "stack memory" So what about to omit "stack" word? Let's write it "Prevent from user malicious access to physical memory". Technically, it is right, but doesn't make any sense, exactly as "stack memory", but who cares as long as it sounds right. Jason, whatever, the change is fine by me and the code is more important to me than proper commit message. Thanks > > Jason
On Sun, Aug 12, 2018 at 08:31:33PM +0300, Leon Romanovsky wrote: > > > Userspace simply see garbage memory which belongs to > > > mlx5_ib_create_qp_resp and not to "stack memory". > > > > mlx5_ib_create_qp_resp is allocated on the stack, so it is properly > > called kernel "stack memory" > > So what about to omit "stack" word? Let's write it "Prevent from user > malicious access to physical memory". In this security community "stack memory leak' is the correct phrasology to refer to this bug class. https://lwn.net/Articles/748642/ Jason
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@vger.kernel.org> > > Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> > > 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@mellanox.com> 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@vger.kernel.org> Acked-by: Leon Romanovsky <leonro@mellanox.com> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> 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
diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c index 6efd770797d121..5839ac0083fa07 100644 --- a/drivers/infiniband/hw/mlx5/qp.c +++ 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; struct mlx5_ib_cq *send_cq; struct mlx5_ib_cq *recv_cq; unsigned long flags; @@ -1763,7 +1761,7 @@ static int create_qp_common(struct mlx5_ib_dev *dev, struct ib_pd *pd, return -EINVAL; } err = create_user_qp(dev, pd, qp, udata, init_attr, &in, - &resp, &inlen, base); + &inlen, base); if (err) mlx5_ib_dbg(dev, "err %d\n", err); } else {
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@vger.kernel.org> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com> --- drivers/infiniband/hw/mlx5/qp.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)