| Message ID | 20140328082428.GH25192@mwanda (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
Hi, Le vendredi 28 mars 2014 à 11:24 +0300, Dan Carpenter a écrit : > The c4iw_alloc_ucontext_resp struct has a 4 byte hole after the last > member and we should clear it before passing it to the user. > > Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes') > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > It's not the proper fix for this issue: an explicit padding has to be added (and initialized), see "Re: [PATCH net-next 2/2] cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes" http://marc.info/?i=1395848977.3297.15.camel@localhost.localdomain In its current form, the c4iw_alloc_ucontext_resp structure does not require padding on i386, so a 32bits userspace program using this structure against a x86_64 kernel will make the kernel do a buffer overflow in userspace, likely on stack, as answer of a GET_CONTEXT request: #include <infiniband/verbs.h> #include <infiniband/kern-abi.h> #define IBV_INIT_CMD_RESP(cmd, size, opcode, out, outsize) \ do { \ (cmd)->command = IB_USER_VERBS_CMD_##opcode; \ (cmd)->in_words = (size) / 4; \ (cmd)->out_words = (outsize) / 4; \ (cmd)->response = (out); \ } while (0) struct c4iw_alloc_ucontext_resp { struct ibv_get_context_resp ibv_resp; __u64 status_page_key; __u32 status_page_size; }; struct ibv_context *alloc_context(struct ibv_device *ibdev, int cmd_fd) { struct ibv_get_context cmd; struct c4iw_alloc_ucontext_resp resp; ssize_t sret; ... IBV_INIT_CMD_RESP(&cmd, sizeof(cmd), GET_CONTEXT, &resp, sizeof(resp)); ... sret = write(context->cmd_fd, &cmd, sizeof(cmd)); if (sret != sizeof(cmd)) { int err = errno; fprintf(stderr, "GET_CONTEXT failed: %d (%s)\n", err, strerror(err)); ... } ... } Unfortunately, it's not the only structure which has this problem. I'm currently preparing a report on this issue for this driver (cxgb4) and another. > diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c > index e36d2a2..a72aaa7 100644 > --- a/drivers/infiniband/hw/cxgb4/provider.c > +++ b/drivers/infiniband/hw/cxgb4/provider.c > @@ -107,7 +107,7 @@ static struct ib_ucontext *c4iw_alloc_ucontext(struct ib_device *ibdev, > struct c4iw_ucontext *context; > struct c4iw_dev *rhp = to_c4iw_dev(ibdev); > static int warned; > - struct c4iw_alloc_ucontext_resp uresp; > + struct c4iw_alloc_ucontext_resp uresp = {}; > int ret = 0; > struct c4iw_mm_entry *mm = NULL; > Regards.
From: Yann Droneaud > Hi, > > Le vendredi 28 mars 2014 11:24 +0300, Dan Carpenter a crit : > > The c4iw_alloc_ucontext_resp struct has a 4 byte hole after the last > > member and we should clear it before passing it to the user. > > > > Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes') > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > It's not the proper fix for this issue: an explicit padding has to be > added (and initialized), see "Re: [PATCH net-next 2/2] cxgb4/iw_cxgb4: > Doorbell Drop Avoidance Bug Fixes" > http://marc.info/?i=1395848977.3297.15.camel@localhost.localdomain > > In its current form, the c4iw_alloc_ucontext_resp structure does not > require padding on i386, so a 32bits userspace program using this > structure against a x86_64 kernel will make the kernel do a buffer > overflow in userspace, likely on stack, as answer of a GET_CONTEXT > request: ... > struct c4iw_alloc_ucontext_resp { > struct ibv_get_context_resp ibv_resp; > __u64 status_page_key; > __u32 status_page_size; > }; Or add __attribute__((aligned(4))) to the 64bit fields. And maybe a compile time assert on the length of the structure. Since it is part of an ABI it must not change David
On Fri, Mar 28, 2014 at 11:27:48AM +0100, Yann Droneaud wrote: > Unfortunately, it's not the only structure which has this problem. I'm > currently preparing a report on this issue for this driver (cxgb4) and > another. > This information leak is still present in linux-next. These days we count those things as security vulnerabilities with CVEs and everything. When is it likely to get fixed? regards, dan carpenter -- 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,
Please find 4 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.
Additionally, as reported by Dan Carpenter, 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. Other are modified to not write the padding field in
response to make the kernel able to handle gracefully i386 userspace on x86_64.
For full coherency, patches against the userspace libraries (libcxgb4 and
libmlx5) will be submitted as a followup to update the data structure on
userspace side.
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 | 8 ++++++--
drivers/infiniband/hw/cxgb4/provider.c | 8 ++++++--
drivers/infiniband/hw/cxgb4/user.h | 2 ++
drivers/infiniband/hw/mlx5/cq.c | 13 +++++++++++--
drivers/infiniband/hw/mlx5/srq.c | 18 +++++++++++++++---
drivers/infiniband/hw/mlx5/user.h | 2 ++
6 files changed, 42 insertions(+), 9 deletions(-)
Hi, Please find two patches for libcxgb4 to keep the library in sync regarding the kernel changes I've submitted: explicit padding must be added at end of the structure so that i386 userspace library has the same structure layout than x86_64 kernel. See http://marc.info/?i=cover.1399216475.git.ydroneaud@opteya.com Yann Droneaud (2): kernel abi: adds explicit padding in struct c4iw_create_cq_resp kernel abi: adds explicit padding in struct c4iw_alloc_ucontext_resp src/cxgb4-abi.h | 2 ++ src/dev.c | 5 +++++ src/verbs.c | 5 +++++ 3 files changed, 12 insertions(+)
Hi, Le samedi 03 mai 2014 à 02:56 +0300, Dan Carpenter a écrit : > On Fri, Mar 28, 2014 at 11:27:48AM +0100, Yann Droneaud wrote: > > Unfortunately, it's not the only structure which has this problem. I'm > > currently preparing a report on this issue for this driver (cxgb4) and > > another. > > > > This information leak is still present in linux-next. These days we > count those things as security vulnerabilities with CVEs and everything. > > When is it likely to get fixed? > Thanks for the reminder. The patches were waiting for some times in my patch queue, I failed to provide them earlier. BTW, I've taken time during the week end to complete them. They should be available on linux-rdma@vger.kernel.org. See http://marc.info/?i=cover.1399216475.git.ydroneaud@opteya.com Regards.
Hi, Le dimanche 04 mai 2014 à 23:21 +0200, Yann Droneaud a écrit : > Please find 4 patches which fix some issues regarding missing explicit > padding at end of structure exchanged between kernel and userspace. > 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. So using pahole I've found issues in mlx5 and cxgb4 only. > These makes i386 userspace libraries and x86_64 kernel disagree about > the size of the structures. > > Additionally, as reported by Dan Carpenter, 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. Other are modified to not write the padding field in > response to make the kernel able to handle gracefully i386 userspace on x86_64. > > For full coherency, patches against the userspace libraries (libcxgb4 and > libmlx5) will be submitted as a followup to update the data structure on > userspace side. > BTW, as I don't have the hardware / I don't have access to the hardware, the patches are not tested against real world. (I only have HCAs handled by mlx4 and qib drivers). Regards.
Applied. Thanks! Steve. -- 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
diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c index e36d2a2..a72aaa7 100644 --- a/drivers/infiniband/hw/cxgb4/provider.c +++ b/drivers/infiniband/hw/cxgb4/provider.c @@ -107,7 +107,7 @@ static struct ib_ucontext *c4iw_alloc_ucontext(struct ib_device *ibdev, struct c4iw_ucontext *context; struct c4iw_dev *rhp = to_c4iw_dev(ibdev); static int warned; - struct c4iw_alloc_ucontext_resp uresp; + struct c4iw_alloc_ucontext_resp uresp = {}; int ret = 0; struct c4iw_mm_entry *mm = NULL;
The c4iw_alloc_ucontext_resp struct has a 4 byte hole after the last member and we should clear it before passing it to the user. Fixes: 05eb23893c2c ('cxgb4/iw_cxgb4: Doorbell Drop Avoidance Bug Fixes') Signed-off-by: Dan Carpenter <dan.carpenter@oracle.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