uverbs ABI fixes
cover uverbs ABI fixes
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(-)
Signature · 6 lines
1.9.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): cover.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
PATCH 1/4 RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_cq
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.
--- drivers/infiniband/hw/mlx5/cq.c | 13 ++++++++++++- drivers/infiniband/hw/mlx5/user.h | 1 + 2 files changed, 13 insertions(+), 1 deletion(-)
drivers/infiniband/hw/mlx5/cq.c +12 -1
@@ -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 ;
drivers/infiniband/hw/mlx5/user.h +1 -0
@@ -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 {
Signature · 6 lines
1.9.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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
Acked by: Eli Cohen <[email protected]>
Re: [PATCHv1 1/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_cq
Hi Roland,
Quoted 2 lines · Eli Cohen
Acked by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>
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.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/4 RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq
In reply to a message not in the archivei386 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.oThen, 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.
--- drivers/infiniband/hw/mlx5/srq.c | 14 +++++++++++++- drivers/infiniband/hw/mlx5/user.h | 1 + 2 files changed, 14 insertions(+), 1 deletion(-)
drivers/infiniband/hw/mlx5/srq.c +13 -1
@@ -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,
drivers/infiniband/hw/mlx5/user.h +1 -0
@@ -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 {
Signature · 6 lines
1.9.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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
Acked by: Eli Cohen <[email protected]>
Re: [PATCHv1 2/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq
Hi,
Quoted 1 line · Eli Cohen
Acked by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>
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.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1401953883.20659.15.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Re: [PATCHv1 2/4] RDMA/mlx5: add missing padding at end of struct mlx5_ib_create_srq
Quoted 8 lines · Yann Droneaud
Quoted 1 line · Eli Cohen
Acked by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/[email protected]>
This Acked-by: tag was not applied on commit
43bc889380c2, in for-next branch (HEAD atca846ce98ca3).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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 4/4 RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp
In reply to a message not in the archivei386 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.oThen, 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.
--- drivers/infiniband/hw/cxgb4/provider.c | 5 +++-- drivers/infiniband/hw/cxgb4/user.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-)
drivers/infiniband/hw/cxgb4/provider.c +3 -2
@@ -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;
drivers/infiniband/hw/cxgb4/user.h +1 -0
@@ -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
Signature · 6 lines
1.9.0 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): b08b42d735d0a9d573ed09f9a30338686a802da0.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Steve Wise
Acked-byRe: [PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] 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
In reply to a message not in the archiveHi Roland,
Quoted 54 lines · 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_respget 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.oThen, 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_64kernel try to write past the buffer provided by an i386 binary.When boundary check will be implemented, the
x86_64kernel 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, like92b0ca7cb149('IB/mlx5: Fix stack info leak inmlx5_ib_alloc_ucontext()'), makes functionc4iw_alloc_ucontext()not writting this padding field to userspace. This way,x86_64kernel will be able to write structc4iw_alloc_ucontext_respas expected by unpatched and patched i386 libcxgb4.
Quoted 7 lines
Fixes 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')Reported-by Yann Droneaud <[email protected]>Reported-by Dan Carpenter <[email protected]>Signed-off-by Yann Droneaud <[email protected]>
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.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1400739665.21049.5.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Re: [PATCHv1 4/4] RDMA/cxgb4: add missing padding at end of struct c4iw_alloc_ucontext_resp
Hi Roland,
Quoted 63 lines · Yann Droneaud
Quoted 54 lines · 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_respget 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.oThen, 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_64kernel try to write past the buffer provided by an i386 binary.When boundary check will be implemented, the
x86_64kernel 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, like92b0ca7cb149('IB/mlx5: Fix stack info leak inmlx5_ib_alloc_ucontext()'), makes functionc4iw_alloc_ucontext()not writting this padding field to userspace. This way,x86_64kernel will be able to write structc4iw_alloc_ucontext_respas expected by unpatched and patched i386 libcxgb4.Quoted 7 lines
Fixes 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes')Reported-by Yann Droneaud <[email protected]>Reported-by Dan Carpenter <[email protected]>Signed-off-by Yann Droneaud <[email protected]>
Quoted 8 lines
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@mwandaThe 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:
<[email protected]> to this patch the next time you rebuild your 'for-next' branch ?
Thanks a lot.
Regards.
Signature · 8 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/4 RDMA/cxgb4: add missing padding at end of struct c4iw_create_cq_resp
In reply to Yann Droneaudi386 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.oThen, 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.
--- drivers/infiniband/hw/cxgb4/cq.c | 4 ++-- drivers/infiniband/hw/cxgb4/user.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-)
drivers/infiniband/hw/cxgb4/cq.c +2 -2
@@ -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;
drivers/infiniband/hw/cxgb4/user.h +1 -0
@@ -48,6 +48,7 @@ struct c4iw_create_cq_resp {__u32 cqid;__u32 size;__u32 qid_mask; +__u32 reserved;/* explicit padding (optional for i386) */ };
Signature · 1 line
1.9.0
A message that is not in the archive (sent off-list or not yet received): 3fd6186b9729d69a3d8431bb5e0461eee1515484.1399309513.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Steve Wise
Acked-byRe: [PATCHv1 3/4] RDMA/cxgb4: add missing padding at end of struct c4iw_create_cq_resp
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html