| Message ID | 20220203180936.GA28699@kili (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RDMA/ucma: fix a kernel-infoleak in ucma_init_qp_attr() | expand |
On Thu, Feb 03, 2022 at 09:14:47PM +0300, Dan Carpenter wrote: > From: Haimin Zhang <tcs.kernel@gmail.com> > > The ib_copy_ah_attr_to_user() function only initializes "resp.grh" if > the "resp.is_global" flag is set. Unfortunately, this data is copied to > the user and copying uninitialized stack data to the user is an > information leak. Zero out the whole struct to be safe. > > Fixes: 4ba66093bdc6 ("IB/core: Check for global flag when using ah_attr") > Reported-by: TCS Robot <tcs_robot@tencent.com> > Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > Resending through the regular lists. > > I added parentheses around the sizeof to make checkpatch happy. > s/sizeof resp/sizeof(resp)/. > > drivers/infiniband/core/ucma.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) The change is ok, but I prefer to initialize to zero as early as possible. diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index 2b72c4fa9550..6d801ed2e46b 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1211,9 +1211,9 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, int in_len, int out_len) { struct rdma_ucm_init_qp_attr cmd; - struct ib_uverbs_qp_attr resp; + struct ib_uverbs_qp_attr resp = {}; struct ucma_context *ctx; - struct ib_qp_attr qp_attr; + struct ib_qp_attr qp_attr = {}; int ret; if (out_len < sizeof(resp)) @@ -1229,8 +1229,6 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, if (IS_ERR(ctx)) return PTR_ERR(ctx); - resp.qp_attr_mask = 0; - memset(&qp_attr, 0, sizeof qp_attr); qp_attr.qp_state = cmd.qp_state; mutex_lock(&ctx->mutex); ret = rdma_init_qp_attr(ctx->cm_id, &qp_attr, &resp.qp_attr_mask); > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > index 9d6ac9dff39a..91485f13d842 100644 > --- a/drivers/infiniband/core/ucma.c > +++ b/drivers/infiniband/core/ucma.c > @@ -1232,7 +1232,7 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, > if (IS_ERR(ctx)) > return PTR_ERR(ctx); > > - resp.qp_attr_mask = 0; > + memset(&resp, 0, sizeof(resp)); > memset(&qp_attr, 0, sizeof qp_attr); > qp_attr.qp_state = cmd.qp_state; > mutex_lock(&ctx->mutex); > -- > 2.20.1 >
On Thu, Feb 03, 2022 at 08:26:16PM +0200, Leon Romanovsky wrote: > On Thu, Feb 03, 2022 at 09:14:47PM +0300, Dan Carpenter wrote: > > From: Haimin Zhang <tcs.kernel@gmail.com> > > > > The ib_copy_ah_attr_to_user() function only initializes "resp.grh" if > > the "resp.is_global" flag is set. Unfortunately, this data is copied to > > the user and copying uninitialized stack data to the user is an > > information leak. Zero out the whole struct to be safe. > > > > Fixes: 4ba66093bdc6 ("IB/core: Check for global flag when using ah_attr") > > Reported-by: TCS Robot <tcs_robot@tencent.com> > > Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com> > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > Resending through the regular lists. > > > > I added parentheses around the sizeof to make checkpatch happy. > > s/sizeof resp/sizeof(resp)/. > > > > drivers/infiniband/core/ucma.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > The change is ok, but I prefer to initialize to zero as early as possible. > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > index 2b72c4fa9550..6d801ed2e46b 100644 > --- a/drivers/infiniband/core/ucma.c > +++ b/drivers/infiniband/core/ucma.c > @@ -1211,9 +1211,9 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, > int in_len, int out_len) > { > struct rdma_ucm_init_qp_attr cmd; > - struct ib_uverbs_qp_attr resp; > + struct ib_uverbs_qp_attr resp = {}; > struct ucma_context *ctx; > - struct ib_qp_attr qp_attr; > + struct ib_qp_attr qp_attr = {}; Will that catch all of the padding in the structure? This seems to come up a lot and I never remember... thanks, greg k-h
On Thu, Feb 03, 2022 at 07:30:57PM +0100, Greg KH wrote: > On Thu, Feb 03, 2022 at 08:26:16PM +0200, Leon Romanovsky wrote: > > On Thu, Feb 03, 2022 at 09:14:47PM +0300, Dan Carpenter wrote: > > > From: Haimin Zhang <tcs.kernel@gmail.com> > > > > > > The ib_copy_ah_attr_to_user() function only initializes "resp.grh" if > > > the "resp.is_global" flag is set. Unfortunately, this data is copied to > > > the user and copying uninitialized stack data to the user is an > > > information leak. Zero out the whole struct to be safe. > > > > > > Fixes: 4ba66093bdc6 ("IB/core: Check for global flag when using ah_attr") > > > Reported-by: TCS Robot <tcs_robot@tencent.com> > > > Signed-off-by: Haimin Zhang <tcs.kernel@gmail.com> > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Resending through the regular lists. > > > > > > I added parentheses around the sizeof to make checkpatch happy. > > > s/sizeof resp/sizeof(resp)/. > > > > > > drivers/infiniband/core/ucma.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > The change is ok, but I prefer to initialize to zero as early as possible. > > > > diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c > > index 2b72c4fa9550..6d801ed2e46b 100644 > > +++ b/drivers/infiniband/core/ucma.c > > @@ -1211,9 +1211,9 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, > > int in_len, int out_len) > > { > > struct rdma_ucm_init_qp_attr cmd; > > - struct ib_uverbs_qp_attr resp; > > + struct ib_uverbs_qp_attr resp = {}; > > struct ucma_context *ctx; > > - struct ib_qp_attr qp_attr; > > + struct ib_qp_attr qp_attr = {}; > > Will that catch all of the padding in the structure? This seems to come > up a lot and I never remember... Yes, last time you asked we went over it. Jason
diff --git a/drivers/infiniband/core/ucma.c b/drivers/infiniband/core/ucma.c index 9d6ac9dff39a..91485f13d842 100644 --- a/drivers/infiniband/core/ucma.c +++ b/drivers/infiniband/core/ucma.c @@ -1232,7 +1232,7 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file, if (IS_ERR(ctx)) return PTR_ERR(ctx); - resp.qp_attr_mask = 0; + memset(&resp, 0, sizeof(resp)); memset(&qp_attr, 0, sizeof qp_attr); qp_attr.qp_state = cmd.qp_state; mutex_lock(&ctx->mutex);