| Message ID | b08b42d735d0a9d573ed09f9a30338686a802da0.1399309513.git.ydroneaud@opteya.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Acked-by: Steve Wise <swise@opengridcomputing.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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@opteya.com > Link: http://marc.info/?i=1395848977.3297.15.camel@localhost.localdomain > Link: http://marc.info/?i=20140328082428.GH25192@mwanda > Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes') > Reported-by: Yann Droneaud <ydroneaud@opteya.com> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> 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.
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@localhost.localdomain > > Link: http://marc.info/?i=20140328082428.GH25192@mwanda > > Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes') > > Reported-by: Yann Droneaud <ydroneaud@opteya.com> > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > > Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> > > 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@vger.kernel.org> to this patch the next time you rebuild your 'for-next' branch ? Cc: <stable@vger.kernel.org> Thanks a lot. Regards.
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
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@localhost.localdomain Link: http://marc.info/?i=20140328082428.GH25192@mwanda Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes') Reported-by: Yann Droneaud <ydroneaud@opteya.com> Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> --- drivers/infiniband/hw/cxgb4/provider.c | 5 +++-- drivers/infiniband/hw/cxgb4/user.h | 1 + 2 files changed, 4 insertions(+), 2 deletions(-)