* [PATCH] infiniband: core: fix information leak to userland
@ 2010-11-06 14:41 Vasiliy Kulikov
2010-11-11 0:01 ` Roland Dreier
0 siblings, 1 reply; 7+ messages in thread
From: Vasiliy Kulikov @ 2010-11-06 14:41 UTC (permalink / raw)
To: kernel-janitors
Cc: Roland Dreier, Sean Hefty, Hal Rosenstock, Alex Chiang,
Andi Kleen, Greg Kroah-Hartman, Julia Lawall, linux-rdma,
linux-kernel
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@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] infiniband: core: fix information leak to userland
2010-11-06 14:41 [PATCH] infiniband: core: fix information leak to userland Vasiliy Kulikov
@ 2010-11-11 0:01 ` Roland Dreier
2010-11-11 0:39 ` Hefty, Sean
2010-11-12 18:08 ` Vasiliy Kulikov
0 siblings, 2 replies; 7+ messages in thread
From: Roland Dreier @ 2010-11-11 0:01 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: kernel-janitors, Roland Dreier, Sean Hefty, Hal Rosenstock,
Alex Chiang, Andi Kleen, Greg Kroah-Hartman, Julia Lawall,
linux-rdma, linux-kernel
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] infiniband: core: fix information leak to userland
2010-11-11 0:01 ` Roland Dreier
@ 2010-11-11 0:39 ` Hefty, Sean
2010-11-12 18:08 ` Vasiliy Kulikov
1 sibling, 0 replies; 7+ messages in thread
From: Hefty, Sean @ 2010-11-11 0:39 UTC (permalink / raw)
To: Roland Dreier, Vasiliy Kulikov
Cc: kernel-janitors, Roland Dreier, Hal Rosenstock, Alex Chiang,
Andi Kleen, Greg Kroah-Hartman, Julia Lawall, linux-rdma,
linux-kernel
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] infiniband: core: fix information leak to userland
2010-11-11 0:01 ` Roland Dreier
2010-11-11 0:39 ` Hefty, Sean
@ 2010-11-12 18:08 ` Vasiliy Kulikov
2010-11-12 18:28 ` Hefty, Sean
1 sibling, 1 reply; 7+ messages in thread
From: Vasiliy Kulikov @ 2010-11-12 18:08 UTC (permalink / raw)
To: Roland Dreier
Cc: kernel-janitors, Roland Dreier, Sean Hefty, Hal Rosenstock,
Alex Chiang, Andi Kleen, Greg Kroah-Hartman, Julia Lawall,
linux-rdma, linux-kernel
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] infiniband: core: fix information leak to userland
2010-11-12 18:08 ` Vasiliy Kulikov
@ 2010-11-12 18:28 ` Hefty, Sean
2010-11-14 9:22 ` [PATCH v2] infiniband: core: fix information leak to userspace Vasiliy Kulikov
0 siblings, 1 reply; 7+ messages in thread
From: Hefty, Sean @ 2010-11-12 18:28 UTC (permalink / raw)
To: Vasiliy Kulikov, Roland Dreier
Cc: kernel-janitors, Roland Dreier, Hal Rosenstock, Alex Chiang,
Andi Kleen, Greg Kroah-Hartman, Julia Lawall, linux-rdma,
linux-kernel
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] infiniband: core: fix information leak to userspace
2010-11-12 18:28 ` Hefty, Sean
@ 2010-11-14 9:22 ` Vasiliy Kulikov
2010-12-02 0:33 ` Roland Dreier
0 siblings, 1 reply; 7+ messages in thread
From: Vasiliy Kulikov @ 2010-11-14 9:22 UTC (permalink / raw)
To: Hefty, Sean
Cc: Roland Dreier, kernel-janitors, Roland Dreier, Hal Rosenstock,
Alex Chiang, Andi Kleen, Greg Kroah-Hartman, Julia Lawall,
linux-rdma, linux-kernel
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@openwall.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] infiniband: core: fix information leak to userspace
2010-11-14 9:22 ` [PATCH v2] infiniband: core: fix information leak to userspace Vasiliy Kulikov
@ 2010-12-02 0:33 ` Roland Dreier
0 siblings, 0 replies; 7+ messages in thread
From: Roland Dreier @ 2010-12-02 0:33 UTC (permalink / raw)
To: Vasiliy Kulikov
Cc: Hefty, Sean, kernel-janitors, Roland Dreier, Hal Rosenstock,
Alex Chiang, Andi Kleen, Greg Kroah-Hartman, Julia Lawall,
linux-rdma, linux-kernel
thanks, applied
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-12-02 0:33 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-06 14:41 [PATCH] infiniband: core: fix information leak to userland Vasiliy Kulikov
2010-11-11 0:01 ` Roland Dreier
2010-11-11 0:39 ` Hefty, Sean
2010-11-12 18:08 ` Vasiliy Kulikov
2010-11-12 18:28 ` Hefty, Sean
2010-11-14 9:22 ` [PATCH v2] infiniband: core: fix information leak to userspace Vasiliy Kulikov
2010-12-02 0:33 ` Roland Dreier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).