[PATCHv1 for v3.15 0/4] uverbs ABI fixes

[PATCHv1 for v3.15 0/4] uverbs ABI fixes

[Yann Droneaud]

Hi,

Please find four updated patches which fix some issues regarding missing
explicit padding at end of structure exchanged between kernel and userspace.

These makes i386 userspace libraries and x86_64 kernel disagree about
the size of the structures.

I've made a review of all others drivers. I've identified the following
structures as part of ABI:

  cxgb3/iw_cxgb3.o     struct iwch_create_cq_req
  cxgb3/iw_cxgb3.o     struct iwch_create_cq_resp
  cxgb3/iw_cxgb3.o     struct iwch_create_qp_resp
  cxgb3/iw_cxgb3.o     struct iwch_reg_user_mr_resp

  cxgb4/iw_cxgb4.o     struct c4iw_alloc_ucontext_resp
  cxgb4/iw_cxgb4.o     struct c4iw_create_cq_resp
  cxgb4/iw_cxgb4.o     struct c4iw_create_qp_resp

  ehca/ib_ehca.o       struct ehca_create_cq_resp
  ehca/ib_ehca.o       struct ehca_create_qp_resp
  ehca/ib_ehca.o       struct ipzu_queue_resp

  mlx4/mlx4_ib.o       struct mlx4_ib_alloc_ucontext_resp
  mlx4/mlx4_ib.o       struct mlx4_ib_alloc_ucontext_resp_v3
  mlx4/mlx4_ib.o       struct mlx4_ib_create_cq
  mlx4/mlx4_ib.o       struct mlx4_ib_create_qp
  mlx4/mlx4_ib.o       struct mlx4_ib_create_srq
  mlx4/mlx4_ib.o       struct mlx4_ib_resize_cq

  mlx5/mlx5_ib.o       struct mlx5_ib_alloc_pd_resp
  mlx5/mlx5_ib.o       struct mlx5_ib_alloc_ucontext_req_v2
  mlx5/mlx5_ib.o       struct mlx5_ib_alloc_ucontext_resp
  mlx5/mlx5_ib.o       struct mlx5_ib_create_cq
  mlx5/mlx5_ib.o       struct mlx5_ib_create_qp
  mlx5/mlx5_ib.o       struct mlx5_ib_create_qp_resp
  mlx5/mlx5_ib.o       struct mlx5_ib_create_srq
  mlx5/mlx5_ib.o       struct mlx5_ib_resize_cq

  mthca/ib_mthca.o     struct mthca_alloc_ucontext_resp
  mthca/ib_mthca.o     struct mthca_create_cq
  mthca/ib_mthca.o     struct mthca_create_qp
  mthca/ib_mthca.o     struct mthca_create_srq
  mthca/ib_mthca.o     struct mthca_reg_mr
  mthca/ib_mthca.o     struct mthca_resize_cq

  nes/iw_nes.o         struct nes_alloc_pd_resp
  nes/iw_nes.o         struct nes_alloc_ucontext_req
  nes/iw_nes.o         struct nes_alloc_ucontext_resp
  nes/iw_nes.o         struct nes_create_cq_req
  nes/iw_nes.o         struct nes_create_cq_resp
  nes/iw_nes.o         struct nes_create_qp_req
  nes/iw_nes.o         struct nes_create_qp_resp
  nes/iw_nes.o         struct nes_mem_reg_req

  ocrdma/ocrdma.o      struct ocrdma_alloc_pd_uresp
  ocrdma/ocrdma.o      struct ocrdma_alloc_ucontext_resp
  ocrdma/ocrdma.o      struct ocrdma_create_cq_ureq
  ocrdma/ocrdma.o      struct ocrdma_create_cq_uresp
  ocrdma/ocrdma.o      struct ocrdma_create_qp_ureq
  ocrdma/ocrdma.o      struct ocrdma_create_qp_uresp
  ocrdma/ocrdma.o      struct ocrdma_create_srq_uresp

  usnic/usnic_verbs.o  struct usnic_ib_create_qp_cmd
  usnic/usnic_verbs.o  struct usnic_ib_create_qp_resp
  usnic/usnic_verbs.o  struct usnic_transport_spec

It seems that amso1100/iw_c2.o, ipath/ib_ipath.o and qib/ib_qib.o don't
make use of structure to exchange data with userspace: they use single
values, either u32 or u64.

I've used 'pahole' to compare structure layout from x86_64 against the one
use for i386 and identified the 4 defects addressed by this patchset:
2 issues in mlx5 and 2 issues in cxgb4 drivers.

Additionally, as reported by Dan Carpenter[1], in one case, stack information
can be leaked by the kernel to userspace due to implicit padding being not
initialized.

Unfortunately, the data structure cannot be fixed alone as it would break
existing applications. So in order to remain compatible with i386 libraries,
providers (hw) functions are modified to use the input length to guess the
expected format of the command in order to check the content of the reserved
field for future usage. Others are modified to not write the padding field in
response to make the kernel able to handle gracefully i386 userspace on x86_64.

Optionnaly, for full coherency, patches against the userspace libraries
(libcxgb4[2] and libmlx5[3]) were submitted to update the data structures
on userspace side.

I haven't be able to test the patches as I don't have (access to)
the required HCAs. For each driver, the patchset should be tested in
3 x 2 setups:
- i386 userspace / i386 kernel,
- i386 userspace / x86_64 kernel,
- x86_64 userspace / x86_64 kernel,
with patched and unpatched userspace library.

Regards.

Changes from v0 [4]:
- simplified handling of non-padded structures in mlx5
- address coding style issues reported by checkpatch.pl
- fixed malformed patches which were hand-edited

[1] http://marc.info/?i=20140328082428.GH25192@mwanda
[2] http://marc.info/?i=cover.1399235229.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[3] http://marc.info/?i=1399239713-18152-1-git-send-email-ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[4] http://marc.info/?i=cover.1399216475.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (4):
  RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_cq
  RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq
  RDMA/cxgb4: add missing padding at end of struct c4iw_create_cq_resp
  RDMA/cxgb4: add missing padding at end of struct
    c4iw_alloc_ucontext_resp

 drivers/infiniband/hw/cxgb4/cq.c       |  4 ++--
 drivers/infiniband/hw/cxgb4/provider.c |  5 +++--
 drivers/infiniband/hw/cxgb4/user.h     |  2 ++
 drivers/infiniband/hw/mlx5/cq.c        | 13 ++++++++++++-
 drivers/infiniband/hw/mlx5/srq.c       | 14 +++++++++++++-
 drivers/infiniband/hw/mlx5/user.h      |  2 ++
 6 files changed, 34 insertions(+), 6 deletions(-)

-- 
1.9.0

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

[PATCHv1 1/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_cq

[Yann Droneaud]

i386 ABI disagree with most other ABIs regarding alignment
of data type larger than 4 bytes: on most ABIs a padding must
be added at end of the structures, while it is not required on
i386.

So for most ABI struct mlx5_ib_create_cq get padded to be aligned
on a 8 bytes multiple, while for i386, such padding is not added.

Tool pahole could be used to find such implicit padding:

  $ pahole --anon_include \
  	 --nested_anon_include \
  	 --recursive \
  	 --class_name mlx5_ib_create_cq \
  	 drivers/infiniband/hw/mlx5/mlx5_ib.o

Then, structure layout can be compared between i386 and x86_64:

  +++ obj-i386/drivers/infiniband/hw/mlx5/mlx5_ib.o.pahole.txt    2014-03-28 11:43:07.386413682 +0100
  --- obj-x86_64/drivers/infiniband/hw/mlx5/mlx5_ib.o.pahole.txt  2014-03-27 13:06:17.788472721 +0100
  @@ -34,9 +34,8 @@ struct mlx5_ib_create_cq {
          __u64                      db_addr;              /*     8     8 */
          __u32                      cqe_size;             /*    16     4 */

  -       /* size: 20, cachelines: 1, members: 3 */
  -       /* last cacheline: 20 bytes */
  +       /* size: 24, cachelines: 1, members: 3 */
  +       /* padding: 4 */
  +       /* last cacheline: 24 bytes */
   };

This ABI disagreement will make an x86_64 kernel try to read
past the buffer provided by an i386 binary.

When boundary check will be implemented, a x86_64 kernel will
refuse to read past the i386 userspace provided buffer
and the uverb will fail.

Anyway, if the structure lay in memory on a page boundary and
next page is not mapped, ib_copy_from_udata() will fail when
trying to read the 4 bytes of padding and the uverb will fail.

This patch makes create_cq_user() takes care of the input
data size to handle the case where no padding is provided.

This way, x86_64 kernel will be able to handle struct mlx5_ib_create_cq
as sent by unpatched and patched i386 libmlx5.

Link: http://marc.info/?i=cover.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Fixes: e126ba97dba9e ('mlx5: Add driver for Mellanox Connect-IB adapter')
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/cq.c   | 13 ++++++++++++-
 drivers/infiniband/hw/mlx5/user.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/cq.c b/drivers/infiniband/hw/mlx5/cq.c
index 62bb6b49dc1d..8ae4f896cb41 100644
--- a/drivers/infiniband/hw/mlx5/cq.c
+++ b/drivers/infiniband/hw/mlx5/cq.c
@@ -32,6 +32,7 @@
 
 #include <linux/kref.h>
 #include <rdma/ib_umem.h>
+#include <rdma/ib_user_verbs.h>
 #include "mlx5_ib.h"
 #include "user.h"
 
@@ -602,14 +603,24 @@ static int create_cq_user(struct mlx5_ib_dev *dev, struct ib_udata *udata,
 			  int *cqe_size, int *index, int *inlen)
 {
 	struct mlx5_ib_create_cq ucmd;
+	size_t ucmdlen;
 	int page_shift;
 	int npages;
 	int ncont;
 	int err;
 
-	if (ib_copy_from_udata(&ucmd, udata, sizeof(ucmd)))
+	ucmdlen =
+		(udata->inlen - sizeof(struct ib_uverbs_cmd_hdr) <
+		 sizeof(ucmd)) ? (sizeof(ucmd) -
+				  sizeof(ucmd.reserved)) : sizeof(ucmd);
+
+	if (ib_copy_from_udata(&ucmd, udata, ucmdlen))
 		return -EFAULT;
 
+	if (ucmdlen == sizeof(ucmd) &&
+	    ucmd.reserved != 0)
+		return -EINVAL;
+
 	if (ucmd.cqe_size != 64 && ucmd.cqe_size != 128)
 		return -EINVAL;
 
diff --git a/drivers/infiniband/hw/mlx5/user.h b/drivers/infiniband/hw/mlx5/user.h
index 0f4f8e42a17f..d44ecd2c2faf 100644
--- a/drivers/infiniband/hw/mlx5/user.h
+++ b/drivers/infiniband/hw/mlx5/user.h
@@ -91,6 +91,7 @@ struct mlx5_ib_create_cq {
 	__u64	buf_addr;
 	__u64	db_addr;
 	__u32	cqe_size;
+	__u32	reserved; /* explicit padding (optional on i386) */
 };
 
 struct mlx5_ib_create_cq_resp {
-- 
1.9.0

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

[PATCHv1 2/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq

[Yann Droneaud]

i386 ABI disagree with most other ABIs regarding alignment
of data type larger than 4 bytes: on most ABIs a padding must
be added at end of the structures, while it is not required on
i386.

So for most ABI struct mlx5_ib_create_srq get implicitly padded
to be aligned on a 8 bytes multiple, while for i386, such padding
is not added.

Tool pahole could be used to find such implicit padding:

  $ pahole --anon_include \
           --nested_anon_include \
           --recursive \
           --class_name mlx5_ib_create_srq \
           drivers/infiniband/hw/mlx5/mlx5_ib.o

Then, structure layout can be compared between i386 and x86_64:

  +++ obj-i386/drivers/infiniband/hw/mlx5/mlx5_ib.o.pahole.txt    2014-03-28 11:43:07.386413682 +0100
  --- obj-x86_64/drivers/infiniband/hw/mlx5/mlx5_ib.o.pahole.txt  2014-03-27 13:06:17.788472721 +0100
  @@ -69,7 +68,6 @@ struct mlx5_ib_create_srq {
          __u64                      db_addr;              /*     8     8 */
          __u32                      flags;                /*    16     4 */

  -       /* size: 20, cachelines: 1, members: 3 */
  -       /* last cacheline: 20 bytes */
  +       /* size: 24, cachelines: 1, members: 3 */
  +       /* padding: 4 */
  +       /* last cacheline: 24 bytes */
   };

ABI disagreement will make an x86_64 kernel try to read past
the buffer provided by an i386 binary.

When boundary check will be implemented, the x86_64 kernel will
refuse to read past the i386 userspace provided buffer and the
uverb will fail.

Anyway, if the structure lay in memory on a page boundary and
next page is not mapped, ib_copy_from_udata() will fail and the
uverb will fail.

This patch makes create_srq_user() takes care of the input
data size to handle the case where no padding was provided.

This way, x86_64 kernel will be able to handle struct mlx5_ib_create_srq
as sent by unpatched and patched i386 libmlx5.

Link: http://marc.info/?i=cover.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Fixes: e126ba97dba9e ('mlx5: Add driver for Mellanox Connect-IB adapter')
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/srq.c  | 14 +++++++++++++-
 drivers/infiniband/hw/mlx5/user.h |  1 +
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/mlx5/srq.c b/drivers/infiniband/hw/mlx5/srq.c
index 210b3eaf188a..384af6dec5eb 100644
--- a/drivers/infiniband/hw/mlx5/srq.c
+++ b/drivers/infiniband/hw/mlx5/srq.c
@@ -35,6 +35,7 @@
 #include <linux/mlx5/srq.h>
 #include <linux/slab.h>
 #include <rdma/ib_umem.h>
+#include <rdma/ib_user_verbs.h>
 
 #include "mlx5_ib.h"
 #include "user.h"
@@ -78,16 +79,27 @@ static int create_srq_user(struct ib_pd *pd, struct mlx5_ib_srq *srq,
 {
 	struct mlx5_ib_dev *dev = to_mdev(pd->device);
 	struct mlx5_ib_create_srq ucmd;
+	size_t ucmdlen;
 	int err;
 	int npages;
 	int page_shift;
 	int ncont;
 	u32 offset;
 
-	if (ib_copy_from_udata(&ucmd, udata, sizeof(ucmd))) {
+	ucmdlen =
+		(udata->inlen - sizeof(struct ib_uverbs_cmd_hdr) <
+		 sizeof(ucmd)) ? (sizeof(ucmd) -
+				  sizeof(ucmd.reserved)) : sizeof(ucmd);
+
+	if (ib_copy_from_udata(&ucmd, udata, ucmdlen)) {
 		mlx5_ib_dbg(dev, "failed copy udata\n");
 		return -EFAULT;
 	}
+
+	if (ucmdlen == sizeof(ucmd) &&
+	    ucmd.reserved != 0)
+		return -EINVAL;
+
 	srq->wq_sig = !!(ucmd.flags & MLX5_SRQ_FLAG_SIGNATURE);
 
 	srq->umem = ib_umem_get(pd->uobject->context, ucmd.buf_addr, buf_size,
diff --git a/drivers/infiniband/hw/mlx5/user.h b/drivers/infiniband/hw/mlx5/user.h
index d44ecd2c2faf..d0ba264ac1ed 100644
--- a/drivers/infiniband/hw/mlx5/user.h
+++ b/drivers/infiniband/hw/mlx5/user.h
@@ -110,6 +110,7 @@ struct mlx5_ib_create_srq {
 	__u64	buf_addr;
 	__u64	db_addr;
 	__u32	flags;
+	__u32	reserved; /* explicit padding (optional on i386) */
 };
 
 struct mlx5_ib_create_srq_resp {
-- 
1.9.0

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

[PATCHv1 3/4] RDMA/cxgb4: add missing padding at end of struct c4iw_create_cq_resp

[Yann Droneaud]

i386 ABI disagree with most other ABIs regarding alignment
of data type larger than 4 bytes: on most ABIs a padding must
be added at end of the structures, while it is not required on
i386.

So for most ABI struct c4iw_create_cq_resp get implicitly padded
to be aligned on a 8 bytes multiple, while for i386, such padding
is not added.

Tool pahole could be used to find such implicit padding:

  $ pahole --anon_include \
           --nested_anon_include \
           --recursive \
           --class_name c4iw_create_cq_resp \
           drivers/infiniband/hw/cxgb4/iw_cxgb4.o

Then, structure layout can be compared between i386 and x86_64:

  +++ obj-i386/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt   2014-03-28 11:43:05.547432195 +0100
  --- obj-x86_64/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt 2014-03-28 10:55:10.990133017 +0100
  @@ -14,9 +13,8 @@ struct c4iw_create_cq_resp {
          __u32                      size;                 /*    28     4 */
          __u32                      qid_mask;             /*    32     4 */

  -       /* size: 36, cachelines: 1, members: 6 */
  -       /* last cacheline: 36 bytes */
  +       /* size: 40, cachelines: 1, members: 6 */
  +       /* padding: 4 */
  +       /* last cacheline: 40 bytes */
   };

This ABI disagreement will make an x86_64 kernel try to write past
the buffer provided by an i386 binary.

When boundary check will be implemented, the x86_64 kernel will
refuse to write past the i386 userspace provided buffer
and the uverbs will fail.

If the structure lay in memory on a page boundary and next page
is not mapped, ib_copy_to_udata() will fail and the uverbs
will fail.

This patch adds an explicit padding at end of structure
c4iw_create_cq_resp, and, like 92b0ca7cb149 ('IB/mlx5: Fix stack
info leak in mlx5_ib_alloc_ucontext()'), makes function c4iw_create_cq()
not writting this padding field to userspace. This way, x86_64 kernel
will be able to write struct c4iw_create_cq_resp as expected by unpatched
and patched i386 libcxgb4.

Link: http://marc.info/?i=cover.1399309513.git.ydroneaud@opteya.com
Cc: stable@vger.kernel.org
Fixes: cfdda9d764362 ('RDMA/cxgb4: Add driver for Chelsio T4 RNIC')
Fixes: e24a72a3302a6 ('RDMA/cxgb4: Fix four byte info leak in c4iw_create_cq()')
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 drivers/infiniband/hw/cxgb4/cq.c   | 4 ++--
 drivers/infiniband/hw/cxgb4/user.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/cq.c b/drivers/infiniband/hw/cxgb4/cq.c
index cfaa56ada189..7151a02b4ebb 100644
--- a/drivers/infiniband/hw/cxgb4/cq.c
+++ b/drivers/infiniband/hw/cxgb4/cq.c
@@ -940,7 +940,6 @@ struct ib_cq *c4iw_create_cq(struct ib_device *ibdev, int entries,
 		if (!mm2)
 			goto err4;
 
-		memset(&uresp, 0, sizeof(uresp));
 		uresp.qid_mask = rhp->rdev.cqmask;
 		uresp.cqid = chp->cq.cqid;
 		uresp.size = chp->cq.size;
@@ -951,7 +950,8 @@ struct ib_cq *c4iw_create_cq(struct ib_device *ibdev, int entries,
 		uresp.gts_key = ucontext->key;
 		ucontext->key += PAGE_SIZE;
 		spin_unlock(&ucontext->mmap_lock);
-		ret = ib_copy_to_udata(udata, &uresp, sizeof uresp);
+		ret = ib_copy_to_udata(udata, &uresp,
+				       sizeof(uresp) - sizeof(uresp.reserved));
 		if (ret)
 			goto err5;
 
diff --git a/drivers/infiniband/hw/cxgb4/user.h b/drivers/infiniband/hw/cxgb4/user.h
index 11ccd276e5d9..9b7534b5f07d 100644
--- a/drivers/infiniband/hw/cxgb4/user.h
+++ b/drivers/infiniband/hw/cxgb4/user.h
@@ -48,6 +48,7 @@ struct c4iw_create_cq_resp {
 	__u32 cqid;
 	__u32 size;
 	__u32 qid_mask;
+	__u32 reserved; /* explicit padding (optional for i386) */
 };
 
 
-- 
1.9.0

[PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp

[Yann Droneaud]

i386 ABI disagree with most other ABIs regarding alignment
of data type larger than 4 bytes: on most ABIs a padding must
be added at end of the structures, while it is not required on
i386.

So for most ABI struct c4iw_alloc_ucontext_resp get implicitly padded
to be aligned on a 8 bytes multiple, while for i386, such padding
is not added.

Tool pahole could be used to find such implicit padding:

  $ pahole --anon_include \
           --nested_anon_include \
           --recursive \
           --class_name c4iw_alloc_ucontext_resp \
           drivers/infiniband/hw/cxgb4/iw_cxgb4.o

Then, structure layout can be compared between i386 and x86_64:

  +++ obj-i386/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt   2014-03-28 11:43:05.547432195 +0100
  --- obj-x86_64/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt 2014-03-28 10:55:10.990133017 +0100
  @@ -2,9 +2,8 @@ struct c4iw_alloc_ucontext_resp {
          __u64                      status_page_key;      /*     0     8 */
          __u32                      status_page_size;     /*     8     4 */

  -       /* size: 12, cachelines: 1, members: 2 */
  -       /* last cacheline: 12 bytes */
  +       /* size: 16, cachelines: 1, members: 2 */
  +       /* padding: 4 */
  +       /* last cacheline: 16 bytes */
   };

This ABI disagreement will make an x86_64 kernel try to write
past the buffer provided by an i386 binary.

When boundary check will be implemented, the x86_64 kernel will
refuse to write past the i386 userspace provided buffer
and the uverbs will fail.

If the structure lay in memory on a page boundary and next page
is not mapped, ib_copy_to_udata() will fail and the uverb
will fail.

Additionally, as reported by Dan Carpenter, without the implicit
padding being properly cleared, an information leak would take
place in most architectures.

This patch adds an explicit padding to struct c4iw_alloc_ucontext_resp,
and, like 92b0ca7cb149 ('IB/mlx5: Fix stack info leak in
mlx5_ib_alloc_ucontext()'), makes function c4iw_alloc_ucontext()
not writting this padding field to userspace. This way, x86_64 kernel
will be able to write struct c4iw_alloc_ucontext_resp as expected by
unpatched and patched i386 libcxgb4.

Link: http://marc.info/?i=cover.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://marc.info/?i=1395848977.3297.15.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org
Link: http://marc.info/?i=20140328082428.GH25192@mwanda
Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')
Reported-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/cxgb4/provider.c | 5 +++--
 drivers/infiniband/hw/cxgb4/user.h     | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
index a94a3e12c349..c777e22bd8d5 100644
--- a/drivers/infiniband/hw/cxgb4/provider.c
+++ b/drivers/infiniband/hw/cxgb4/provider.c
@@ -122,7 +122,7 @@ static struct ib_ucontext *c4iw_alloc_ucontext(struct ib_device *ibdev,
 	INIT_LIST_HEAD(&context->mmaps);
 	spin_lock_init(&context->mmap_lock);
 
-	if (udata->outlen < sizeof(uresp)) {
+	if (udata->outlen < sizeof(uresp) - sizeof(uresp.reserved)) {
 		if (!warned++)
 			pr_err(MOD "Warning - downlevel libcxgb4 (non-fatal), device status page disabled.");
 		rhp->rdev.flags |= T4_STATUS_PAGE_DISABLED;
@@ -140,7 +140,8 @@ static struct ib_ucontext *c4iw_alloc_ucontext(struct ib_device *ibdev,
 		context->key += PAGE_SIZE;
 		spin_unlock(&context->mmap_lock);
 
-		ret = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
+		ret = ib_copy_to_udata(udata, &uresp,
+				       sizeof(uresp) - sizeof(uresp.reserved));
 		if (ret)
 			goto err_mm;
 
diff --git a/drivers/infiniband/hw/cxgb4/user.h b/drivers/infiniband/hw/cxgb4/user.h
index 9b7534b5f07d..cbd0ce170728 100644
--- a/drivers/infiniband/hw/cxgb4/user.h
+++ b/drivers/infiniband/hw/cxgb4/user.h
@@ -75,5 +75,6 @@ struct c4iw_create_qp_resp {
 struct c4iw_alloc_ucontext_resp {
 	__u64 status_page_key;
 	__u32 status_page_size;
+	__u32 reserved; /* explicit padding (optional for i386) */
 };
 #endif
-- 
1.9.0

--
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: [PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp

[Steve Wise]

Acked-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
--
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: [PATCHv1 3/4] RDMA/cxgb4: add missing padding at end of struct c4iw_create_cq_resp

[Steve Wise]

Acked-by: Steve Wise <swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org>
--
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: [PATCHv1 1/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_cq

[Eli Cohen]

Acked by: Eli Cohen <eli@mellanox.com>

Re: [PATCHv1 2/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq

[Eli Cohen]

Acked by: Eli Cohen <eli@mellanox.com>

Re: [PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp

[Yann Droneaud]

Hi Roland,

Le lundi 05 mai 2014 à 19:35 +0200, Yann Droneaud a écrit :
> i386 ABI disagree with most other ABIs regarding alignment
> of data type larger than 4 bytes: on most ABIs a padding must
> be added at end of the structures, while it is not required on
> i386.
> 
> So for most ABI struct c4iw_alloc_ucontext_resp get implicitly padded
> to be aligned on a 8 bytes multiple, while for i386, such padding
> is not added.
> 
> Tool pahole could be used to find such implicit padding:
> 
>   $ pahole --anon_include \
>            --nested_anon_include \
>            --recursive \
>            --class_name c4iw_alloc_ucontext_resp \
>            drivers/infiniband/hw/cxgb4/iw_cxgb4.o
> 
> Then, structure layout can be compared between i386 and x86_64:
> 
>   +++ obj-i386/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt   2014-03-28 11:43:05.547432195 +0100
>   --- obj-x86_64/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt 2014-03-28 10:55:10.990133017 +0100
>   @@ -2,9 +2,8 @@ struct c4iw_alloc_ucontext_resp {
>           __u64                      status_page_key;      /*     0     8 */
>           __u32                      status_page_size;     /*     8     4 */
> 
>   -       /* size: 12, cachelines: 1, members: 2 */
>   -       /* last cacheline: 12 bytes */
>   +       /* size: 16, cachelines: 1, members: 2 */
>   +       /* padding: 4 */
>   +       /* last cacheline: 16 bytes */
>    };
> 
> This ABI disagreement will make an x86_64 kernel try to write
> past the buffer provided by an i386 binary.
> 
> When boundary check will be implemented, the x86_64 kernel will
> refuse to write past the i386 userspace provided buffer
> and the uverbs will fail.
> 
> If the structure lay in memory on a page boundary and next page
> is not mapped, ib_copy_to_udata() will fail and the uverb
> will fail.
> 
> Additionally, as reported by Dan Carpenter, without the implicit
> padding being properly cleared, an information leak would take
> place in most architectures.
> 
> This patch adds an explicit padding to struct c4iw_alloc_ucontext_resp,
> and, like 92b0ca7cb149 ('IB/mlx5: Fix stack info leak in
> mlx5_ib_alloc_ucontext()'), makes function c4iw_alloc_ucontext()
> not writting this padding field to userspace. This way, x86_64 kernel
> will be able to write struct c4iw_alloc_ucontext_resp as expected by
> unpatched and patched i386 libcxgb4.
> 
> Link: http://marc.info/?i=cover.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Link: http://marc.info/?i=1395848977.3297.15.camel-bi+AKbBUZKY6gyzm1THtWQ@public.gmane.orgomain
> Link: http://marc.info/?i=20140328082428.GH25192@mwanda
> Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')
> Reported-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

I believe this one should go in v3.15-rc7 as it fixes an issue
introduced in v3.15-rc1. See 
http://marc.info/?i=20140328082428.GH25192@mwanda
http://marc.info/?i=20140502235616.GJ4963@mwanda

The other patchs could probably wait for v3.16-rc1 for integration in
linux-stable.

Regards.

-- 
Yann Droneaud
OPTEYA


--
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: [PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp

[Yann Droneaud]

Hi Roland,

Le jeudi 22 mai 2014 à 08:21 +0200, Yann Droneaud a écrit :
> Le lundi 05 mai 2014 à 19:35 +0200, Yann Droneaud a écrit :
> > i386 ABI disagree with most other ABIs regarding alignment
> > of data type larger than 4 bytes: on most ABIs a padding must
> > be added at end of the structures, while it is not required on
> > i386.
> > 
> > So for most ABI struct c4iw_alloc_ucontext_resp get implicitly padded
> > to be aligned on a 8 bytes multiple, while for i386, such padding
> > is not added.
> > 
> > Tool pahole could be used to find such implicit padding:
> > 
> >   $ pahole --anon_include \
> >            --nested_anon_include \
> >            --recursive \
> >            --class_name c4iw_alloc_ucontext_resp \
> >            drivers/infiniband/hw/cxgb4/iw_cxgb4.o
> > 
> > Then, structure layout can be compared between i386 and x86_64:
> > 
> >   +++ obj-i386/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt   2014-03-28 11:43:05.547432195 +0100
> >   --- obj-x86_64/drivers/infiniband/hw/cxgb4/iw_cxgb4.o.pahole.txt 2014-03-28 10:55:10.990133017 +0100
> >   @@ -2,9 +2,8 @@ struct c4iw_alloc_ucontext_resp {
> >           __u64                      status_page_key;      /*     0     8 */
> >           __u32                      status_page_size;     /*     8     4 */
> > 
> >   -       /* size: 12, cachelines: 1, members: 2 */
> >   -       /* last cacheline: 12 bytes */
> >   +       /* size: 16, cachelines: 1, members: 2 */
> >   +       /* padding: 4 */
> >   +       /* last cacheline: 16 bytes */
> >    };
> > 
> > This ABI disagreement will make an x86_64 kernel try to write
> > past the buffer provided by an i386 binary.
> > 
> > When boundary check will be implemented, the x86_64 kernel will
> > refuse to write past the i386 userspace provided buffer
> > and the uverbs will fail.
> > 
> > If the structure lay in memory on a page boundary and next page
> > is not mapped, ib_copy_to_udata() will fail and the uverb
> > will fail.
> > 
> > Additionally, as reported by Dan Carpenter, without the implicit
> > padding being properly cleared, an information leak would take
> > place in most architectures.
> > 
> > This patch adds an explicit padding to struct c4iw_alloc_ucontext_resp,
> > and, like 92b0ca7cb149 ('IB/mlx5: Fix stack info leak in
> > mlx5_ib_alloc_ucontext()'), makes function c4iw_alloc_ucontext()
> > not writting this padding field to userspace. This way, x86_64 kernel
> > will be able to write struct c4iw_alloc_ucontext_resp as expected by
> > unpatched and patched i386 libcxgb4.
> > 
> > Link: http://marc.info/?i=cover.1399309513.git.ydroneaud@opteya.com
> > Link: http://marc.info/?i=1395848977.3297.15.camel-bi+AKbBUZKaGOdeMZTrFbA@public.gmane.orgldomain
> > Link: http://marc.info/?i=20140328082428.GH25192@mwanda
> > Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')
> > Reported-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> > Reported-by: Dan Carpenter <dan.carpenter-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> 
> I believe this one should go in v3.15-rc7 as it fixes an issue
> introduced in v3.15-rc1. See 
> http://marc.info/?i=20140328082428.GH25192@mwanda
> http://marc.info/?i=20140502235616.GJ4963@mwanda
> 
> The other patchs could probably wait for v3.16-rc1 for integration in
> linux-stable.
> 

I think this patch is likely not going to v3.15, so in order to have it 
integrated in v3.15.x with the others, could you add Cc: 
<stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> to  this patch the next time you rebuild
your 'for-next' branch ?

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>

Thanks a lot.

Regards.

-- 
Yann Droneaud
OPTEYA


--
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: [PATCHv1 1/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_cq

[Yann Droneaud]

Hi Roland,

Le dimanche 11 mai 2014 à 10:10 +0300, Eli Cohen a écrit :
> Acked by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 

This Acked-by: tag was not applied on commit a8237b32a3fa,
in for-next branch (HEAD at ca846ce98ca3).

PS: Eli, you've made a typo in your Acked-by :)

Regards.

-- 
Yann Droneaud
OPTEYA


--
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: [PATCHv1 2/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq

[Yann Droneaud]

Hi,

Le dimanche 11 mai 2014 à 10:12 +0300, Eli Cohen a écrit :
> Acked by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This Acked-by: tag was not applied on commit 43bc889380c2,
in for-next branch (HEAD at ca846ce98ca3).

PS: Eli, you've made a typo in your Acked-by :)

Regards.

-- 
Yann Droneaud
OPTEYA


--
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: [PATCHv1 2/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq

[Eli Cohen]

On Thu, Jun 05, 2014 at 09:38:03AM +0200, Yann Droneaud wrote:
> Le dimanche 11 mai 2014 à 10:12 +0300, Eli Cohen a écrit :
> > Acked by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> This Acked-by: tag was not applied on commit 43bc889380c2,
> in for-next branch (HEAD at ca846ce98ca3).
> 
> PS: Eli, you've made a typo in your Acked-by :)
> 

I forgot the hyphen... sorry :-)
--
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