[PATCH] RDMA/ucma: fix a kernel-infoleak in ucma_init_qp_attr()

[PATCH] RDMA/ucma: fix a kernel-infoleak in ucma_init_qp_attr()

[Dan Carpenter]

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(-)

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

Re: [PATCH] RDMA/ucma: fix a kernel-infoleak in ucma_init_qp_attr()

[Leon Romanovsky]

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
> 

Re: [PATCH] RDMA/ucma: fix a kernel-infoleak in ucma_init_qp_attr()

[Greg KH]

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

Re: [PATCH] RDMA/ucma: fix a kernel-infoleak in ucma_init_qp_attr()

[Jason Gunthorpe]

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