[PATCH] infiniband: core: fix information leak to userland

[PATCH] infiniband: core: fix information leak to userland

[Vasiliy Kulikov]

Structure ib_uverbs_qp_attr is copied to userland with allmost all
fields uninitialized (140 bytes on x86).  It leads to leaking of
contents of kernel stack memory.

Signed-off-by: Vasiliy Kulikov <segooon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 Compile tested.

 drivers/infiniband/core/ucm.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/core/ucm.c b/drivers/infiniband/core/ucm.c
index 08f948d..ccfa4f9 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -622,7 +622,7 @@ static ssize_t ib_ucm_init_qp_attr(struct ib_ucm_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;
 	result = ib_cm_init_qp_attr(ctx->cm_id, &qp_attr, &resp.qp_attr_mask);
-- 
1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: core: fix information leak to userland

[Roland Dreier]

 > Structure ib_uverbs_qp_attr is copied to userland with allmost all
 > fields uninitialized (140 bytes on x86).  It leads to leaking of
 > contents of kernel stack memory.

I don't think most of the fields are uninitialized... we have:

 	memset(&qp_attr, 0, sizeof qp_attr);

and then later on,

	ib_copy_qp_attr_to_user(&resp, &qp_attr);

which actually does initialize almost all of the fields in resp.  The
things that are missing are clearing out the reserved fields in the
structures, and also resp.qp_state never gets set.

I would suggest adding code to clear the reserved fields of structures
to ib_copy_qp_attr_to_user() and ib_copy_ah_attr_to_user(), since this
will fix what looks to be the same problem in ucma_init_qp_attr() (in
drivers/infiniband/core/ucma.c).

Sean, what is intended for qp_state handling here?  It seems
ib_copy_qp_attr_to_user() should either clear it or set it to something
sensible.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] infiniband: core: fix information leak to userland

[Hefty, Sean]

> Sean, what is intended for qp_state handling here?  It seems
> ib_copy_qp_attr_to_user() should either clear it or set it to something
> sensible.

I'm not sure what the original intent was, but both libibcm and librdmacm provide the qp_state as input to the init_qp_attr calls.  It doesn't end up mattering if the kernel returns the value because the corresponding call in libibverbs (ibv_copy_qp_attr_from_kern) doesn't copy out the qp_state.  So, the value that was originally specified ends up being used.  The flow looks something like this:

qp_attr.qp_state = INIT;
cmd.qp_state = qp_attr.qp_state;
write(..cmd..);
ibv_copy_qp_attr_from_kern(&qp_attr, cmd.resp)

I agree that it makes sense for ib_copy_qp_attr_to_user() to set the qp_state.  Deciding what to do in libibverbs seems more troublesome.

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] infiniband: core: fix information leak to userland

[Vasiliy Kulikov]

On Wed, Nov 10, 2010 at 16:01 -0800, Roland Dreier wrote:
>  > Structure ib_uverbs_qp_attr is copied to userland with allmost all
>  > fields uninitialized (140 bytes on x86).  It leads to leaking of
>  > contents of kernel stack memory.
> 
> I don't think most of the fields are uninitialized... we have:
> 
>  	memset(&qp_attr, 0, sizeof qp_attr);
> 
> and then later on,
> 
> 	ib_copy_qp_attr_to_user(&resp, &qp_attr);

Uh, sorry, I was over-pessimistic here...

> which actually does initialize almost all of the fields in resp.  The
> things that are missing are clearing out the reserved fields in the
> structures, and also resp.qp_state never gets set.
> 
> I would suggest adding code to clear the reserved fields of structures
> to ib_copy_qp_attr_to_user() and ib_copy_ah_attr_to_user(), since this
> will fix what looks to be the same problem in ucma_init_qp_attr() (in
> drivers/infiniband/core/ucma.c).


Also part of grh field and ib_uverbs_ah_attr->reserved.  How do you see
this variant of zeroing?  (I don't know whether these fields may be
needed for another callers.)

diff --git a/drivers/infiniband/core/ucm.c
b/drivers/infiniband/core/ucm.c
index 08f948d..f7256f3 100644
--- a/drivers/infiniband/core/ucm.c
+++ b/drivers/infiniband/core/ucm.c
@@ -629,6 +629,7 @@ static ssize_t ib_ucm_init_qp_attr(struct ib_ucm_file *file,
        if (result)
                goto out;
 
+       resp.qp_state = 0;
        ib_copy_qp_attr_to_user(&resp, &qp_attr);
 
        if (copy_to_user((void __user *)(unsigned long)cmd.response,
diff --git a/drivers/infiniband/core/ucma.c
b/drivers/infiniband/core/ucma.c
index ca12acf..07fd247 100644
--- a/drivers/infiniband/core/ucma.c
+++ b/drivers/infiniband/core/ucma.c
@@ -842,6 +842,7 @@ static ssize_t ucma_init_qp_attr(struct ucma_file *file,
        if (ret)
                goto out;
 
+       resp.qp_state = 0;
        ib_copy_qp_attr_to_user(&resp, &qp_attr);
        if (copy_to_user((void __user *)(unsigned long)cmd.response,
                         &resp, sizeof(resp)))
diff --git a/drivers/infiniband/core/uverbs_marshall.c
b/drivers/infiniband/core/uverbs_marshall.c
index 5440da0..cceaf33 100644
--- a/drivers/infiniband/core/uverbs_marshall.c
+++ b/drivers/infiniband/core/uverbs_marshall.c
@@ -35,6 +35,7 @@
 void ib_copy_ah_attr_to_user(struct ib_uverbs_ah_attr *dst, struct ib_ah_attr *src)
 {
+       memset(&dst->grh, 0, sizeof(dst->grh));
        memcpy(dst->grh.dgid, src->grh.dgid.raw, sizeof src->grh.dgid);
        dst->grh.flow_label        = src->grh.flow_label;
        dst->grh.sgid_index        = src->grh.sgid_index;
@@ -46,6 +47,7 @@ void ib_copy_ah_attr_to_user(struct ib_uverbs_ah_attr *dst,
        dst->static_rate           = src->static_rate;
        dst->is_global             = src->ah_flags & IB_AH_GRH ? 1 : 0;
        dst->port_num              = src->port_num;
+       dst->reserved              = 0;
 }
 EXPORT_SYMBOL(ib_copy_ah_attr_to_user);
 
@@ -83,6 +85,7 @@ void ib_copy_qp_attr_to_user(struct ib_uverbs_qp_attr *dst,
        dst->rnr_retry          = src->rnr_retry;
        dst->alt_port_num       = src->alt_port_num;
        dst->alt_timeout        = src->alt_timeout;
+       memset(dst->reserved, 0, sizeof(dst->reserved));
 }
 EXPORT_SYMBOL(ib_copy_qp_attr_to_user);

-- 
Vasiliy
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] infiniband: core: fix information leak to userland

[Hefty, Sean]

> diff --git a/drivers/infiniband/core/ucm.c
> b/drivers/infiniband/core/ucm.c
> index 08f948d..f7256f3 100644
> --- a/drivers/infiniband/core/ucm.c
> +++ b/drivers/infiniband/core/ucm.c
> @@ -629,6 +629,7 @@ static ssize_t ib_ucm_init_qp_attr(struct ib_ucm_file
> *file,
>         if (result)
>                 goto out;
> 
> +       resp.qp_state = 0;
>         ib_copy_qp_attr_to_user(&resp, &qp_attr);

I believe we want ib_copy_qp_attr_to_user() to assign	resp->qp_state = qp_attr->qp_state

> diff --git a/drivers/infiniband/core/ucma.c
> b/drivers/infiniband/core/ucma.c
> index ca12acf..07fd247 100644
> --- a/drivers/infiniband/core/ucma.c
> +++ b/drivers/infiniband/core/ucma.c
> @@ -842,6 +842,7 @@ static ssize_t ucma_init_qp_attr(struct ucma_file
> *file,
>         if (ret)
>                 goto out;
> 
> +       resp.qp_state = 0;
>         ib_copy_qp_attr_to_user(&resp, &qp_attr);
>         if (copy_to_user((void __user *)(unsigned long)cmd.response,
>                          &resp, sizeof(resp)))
> diff --git a/drivers/infiniband/core/uverbs_marshall.c
> b/drivers/infiniband/core/uverbs_marshall.c
> index 5440da0..cceaf33 100644
> --- a/drivers/infiniband/core/uverbs_marshall.c
> +++ b/drivers/infiniband/core/uverbs_marshall.c
> @@ -35,6 +35,7 @@
>  void ib_copy_ah_attr_to_user(struct ib_uverbs_ah_attr *dst, struct
> ib_ah_attr *src)
>  {
> +       memset(&dst->grh, 0, sizeof(dst->grh));

We only need to set dst->grh.reserved = 0.  The other fields are assigned. 

- Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] infiniband: core: fix information leak to userspace

[Vasiliy Kulikov]

ib_ucm_init_qp_attr() and ucma_init_qp_attr() pass struct ib_uverbs_qp_attr
with reserved, qp_state, {ah_attr,alt_ah_attr}{reserved,->grh.reserved}
fields uninitialized to copy_to_user().  It leads to leaking of contents of
kernel stack memory to userspace.

Signed-off-by: Vasiliy Kulikov <segoon-cxoSlKxDwOJWk0Htik3J/w@public.gmane.org>
---
 Compile tested.

 drivers/infiniband/core/uverbs_marshall.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_marshall.c b/drivers/infiniband/core/uverbs_marshall.c
index 5440da0..1b1146f 100644
--- a/drivers/infiniband/core/uverbs_marshall.c
+++ b/drivers/infiniband/core/uverbs_marshall.c
@@ -40,18 +40,21 @@ void ib_copy_ah_attr_to_user(struct ib_uverbs_ah_attr *dst,
 	dst->grh.sgid_index        = src->grh.sgid_index;
 	dst->grh.hop_limit         = src->grh.hop_limit;
 	dst->grh.traffic_class     = src->grh.traffic_class;
+	memset(&dst->grh.reserved, 0, sizeof(dst->grh.reserved));
 	dst->dlid 	    	   = src->dlid;
 	dst->sl   	    	   = src->sl;
 	dst->src_path_bits 	   = src->src_path_bits;
 	dst->static_rate   	   = src->static_rate;
 	dst->is_global             = src->ah_flags & IB_AH_GRH ? 1 : 0;
 	dst->port_num 	    	   = src->port_num;
+	dst->reserved 		   = 0;
 }
 EXPORT_SYMBOL(ib_copy_ah_attr_to_user);
 
 void ib_copy_qp_attr_to_user(struct ib_uverbs_qp_attr *dst,
 			     struct ib_qp_attr *src)
 {
+	dst->qp_state	        = src->qp_state;
 	dst->cur_qp_state	= src->cur_qp_state;
 	dst->path_mtu		= src->path_mtu;
 	dst->path_mig_state	= src->path_mig_state;
@@ -83,6 +86,7 @@ void ib_copy_qp_attr_to_user(struct ib_uverbs_qp_attr *dst,
 	dst->rnr_retry		= src->rnr_retry;
 	dst->alt_port_num	= src->alt_port_num;
 	dst->alt_timeout	= src->alt_timeout;
+	memset(dst->reserved, 0, sizeof(dst->reserved));
 }
 EXPORT_SYMBOL(ib_copy_qp_attr_to_user);
 
-- 
1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] infiniband: core: fix information leak to userspace

[Roland Dreier]

thanks, applied
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html