| Message ID | CAL1RGDUfC38aKqDv7rOnbi3m7PMPC8ze+1kymgrWwNvYHU7e2Q@mail.gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hi Roland, Le jeudi 19 décembre 2013 à 09:08 -0800, Roland Dreier a écrit : > > Unfortunately, some patches in V4 were replacement for patches in V3. > > In this particular case, I've rework the commit messages in V4, so these > > changes might be lost. > > > And I'm a bit disappointed about this particular patch applied with a > > conflict fix: > > > > https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026 > > > > if (!access_ok(VERIFY_WRITE, > > (const void __user *) (unsigned long) ex_hdr.response, > > (hdr.out_words + ex_hdr.provider_out_words) * 8)) > > return -EFAULT; > > > > https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/tree/drivers/infiniband/core/uverbs_main.c?h=for-next&id=2f92baf8313756fd32da4d2a24abc67c8f4f7026#n679 > > > > Checking for write access on a pointer to const doesn't sound right. > > It's harmless, and, if sparse/coccinelle doesn't have a check for such, > > it should be added to report a warning. > > Sorry for that mistake. I've fixed that conflict fix to get rid of > the const. I've also pulled in the changelog updates. > > However, I still prefer not moving all the casts out into every place > where we use INIT_UDATA(), so I'd prefer something like the patch > below. I integrated that into the series and pushed out the result > (since everyone seems OK with rebasing for-next). > > Please let me know what you think. > I'm a bit puzzled :) Sure it address the warning problem from sparse and gcc. It's even handling setting NULL for response if out_words is 0, which was part of another patch[1] in the patchset. So I would say OK. But I'm concerned about this global change of behavior of INIT_UDATA(). I felt it was OK to handle the case of out_words equal to 0 in the new uverbs, since there's no current user of this, there's nothing to broke. It was a limited, locally change. But when changing globally the behavor of INIT_UDATA(), what about existing program which pass a non-NULL address for the response and set out_words to 0 in a command. libibverbs doesn't seems to do this, but who knows how one might try to access to uverbs ? Since currently ib_copy_to_udata() doesn't check struct ib_udata outlen (I have patch for that[2]), some provider (hw/) might be using ib_copy_to_udata() without paying attention to outlen. If outbuf is set to NULL when outlen is 0, then it might broke, and this could be called a regression. I haven't double check each and every user of ib_copy_to_udata(), even if I've found some ugly hack regarding udata, I'm looking at you ipath[3] and qib[4], I haven't payed yet attention to user of ib_copy_to_udata() without check for outlen first. In this other hand, with my patch[2] from the patchset[5], I've done some tests, and haven't found any regression, but my testbed was very limited. So I think we must double check here before applying a global change to INIT_UDATA() and/or be ready to pay the price of regression in some vendor software (I'm not aware of) ... [1] [PATCHv4 for-3.13 04/10] IB/uverbs: set outbuf to NULL when no core response space is provided http://marc.info/?i=330b13a0884b6cc03e287a32f8f49c1aac6bdbed.1387273677.git.ydroneaud@opteya.com [2] [PATCH 04/22] infiniband: ib_copy_to_udata(): check output length http://marc.info/?i=d27716a3a1c180f832d153a7402f65ea8a75b734.1376847403.git.ydroneaud@opteya.com [3] ipath_srq.c:254 ipath_modify_srq() http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/ipath/ipath_srq.c?id=v3.13-rc4#n254 [4] qib_srq.c:250 qib_modify_srq() http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/qib/qib_srq.c?id=v3.13-rc4#n250 [5] [PATCH 00/22] infiniband: improve userspace input check http://marc.info/?i=cover.1376847403.git.ydroneaud@opteya.com Regards.
Hi, Le jeudi 19 décembre 2013 à 19:29 +0100, Yann Droneaud a écrit : > Le jeudi 19 décembre 2013 à 09:08 -0800, Roland Dreier a écrit : > > > > However, I still prefer not moving all the casts out into every place > > where we use INIT_UDATA(), so I'd prefer something like the patch > > below. I integrated that into the series and pushed out the result > > (since everyone seems OK with rebasing for-next). > > > > Please let me know what you think. > > > > I'm a bit puzzled :) Sure it address the warning problem from sparse and > gcc. It's even handling setting NULL for response if out_words is 0, > which was part of another patch[1] in the patchset. So I would say OK. > > But I'm concerned about this global change of behavior of INIT_UDATA(). > > I felt it was OK to handle the case of out_words equal to 0 in the new > uverbs, since there's no current user of this, there's nothing to broke. > It was a limited, locally change. > > But when changing globally the behavor of INIT_UDATA(), what about > existing program which pass a non-NULL address for the response and set > out_words to 0 in a command. > > libibverbs doesn't seems to do this, but who knows how one might try to > access to uverbs ? > > Since currently ib_copy_to_udata() doesn't check struct ib_udata outlen > (I have patch for that[2]), some provider (hw/) might be using > ib_copy_to_udata() without paying attention to outlen. > > If outbuf is set to NULL when outlen is 0, then it might broke, and this > could be called a regression. > > I haven't double check each and every user of ib_copy_to_udata(), even > if I've found some ugly hack regarding udata, I'm looking at you > ipath[3] and qib[4], I haven't payed yet attention to user of > ib_copy_to_udata() without check for outlen first. > I took some time to read again the source code. I've asked about the consequences for users of ib_copy_to_udata(), but ib_copy_from_udata() is also concerned by this issue, but very unlikely. To demonstrate how it is possible to have valid (!) uverbs calls building a struct ib_udata with either outlen set 0 or inlen set 0, let's define some 'vendor' command and response structure in userspace: #include <infiniband/kern-abi.h> struct ibv_cmd_hdr { __u32 command; __u16 in_words; __u16 out_words; }; struct vendor_create_cq { struct ibv_create_cq ibv_cmd; /* vendor defined fields */ }; struct vendor_create_cq_resp resp { struct ibv_create_cq_resp ibv_resp; /* vendor defined fields */ }; Now, let's build a request through a custom 'libibverbs/libvendor-rdma' which might be valid under current kernel, but will set outlen to 0 while response/outbuf would be != NULL: struct ibv_cq *vendor_create_cq_broken_outlen(...) { struct vendor_create_cq cmd; struct vendor_create_cq_resp resp; ... IBV_INIT_CMD_RESP(&cmd, sizeof(cmd), CREATE_CQ, &resp, sizeof(struct ibv_create_cq_resp)); /* <-- */ ... write(context->cmd_fd, &cmd, sizeof(cmd)); ... } Now let's build a request which might be valid with current kernel but will set inlen to 0 while it will be possible for vendor driver (provider, under hw/) to read vendor fields: struct ibv_cq *vendor_create_cq_broken_inlen(...) { struct vendor_create_cq cmd; struct vendor_create_cq_resp resp; size_t cmd_size = sizeof(struct ibv_create_cq) - sizeof(struct ibv_cmd_hdr); /* <-- */ ... assert(cmd_size >= sizeof(struct ibv_cmd_hdr)); ... IBV_INIT_CMD_RESP(&cmd, cmd_size, CREATE_CQ, &resp, sizeof(resp)); ... write(context->cmd_fd, &cmd, cmd_size); ... } It's harder to get it wrong here, because struct ib_udata inlen, for current uverbs (eg. not the extended one, currently only flow steering related), have not subtracted the command header length, while the inbuf pointer is added the command header length. This is a bit schizophrenic. It's making bound checking a little bit harder, see for example mthca_reg_user_mr(), which is the only function doing it right. http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/hw/mthca/mthca_provider.c?id=v3.13-rc4#n988 (I also have a patch to subtract the size of header from input length, so that my patches[1] to add proper bound checking become effective at checking the bounds). So in order to trigger INIT_UDATA() to set inbuf to NULL, the total size of the command must be equal to the size of command, leaving apart the size of the header, while still required to be greater than the header to be accepted by ib_uverbs_write(). We can safely state that it is very unlikely that any unknown userspace program is doing this. But keep it mind it's possible. So if INIT_UDATA() is changed to set inbuf to NULL if inlen is equal to 0, and to set outbuf to NULL if outlen is equal to 0, then, code using, respectively, ib_copy_from_udata() and ib_copy_to_udata() without checking the size will break for the previous code snippet. I believe these are corner cases, and, in french, I would say it's "tiré par les cheveux" [2], but when looking at infiniband/hw/, we can see several (hint, *a lot of*) places where bound checking is not done. (That's why I've suggested adding checks in ib_copy_{from,to}_udata(), in my patchset[1][3][4], but it require subtracting the header length, fix mthca to not do it, fix ipath and qib to not swap the pointer behind the scene, and a lot of testing/reviewing to catch possible regression, but sometime these a desirable, but I digress). Here's a list of calls to ib_copy_{from,to}_data() for which there's no or incorrect checking of {input,output} length: - drivers/infiniband/hw/cxgb3/iwch_provider.c:162, iwch_create_cq() ib_copy_from_udata() with no check on input length. - drivers/infiniband/hw/cxgb3/iwch_provider.c:232, iwch_create_cq() ib_copy_to_udata(), with a check on output length, but will copy even if output length is 0. - drivers/infiniband/hw/cxgb3/iwch_provider.c:436, iwch_allocate_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/cxgb3/iwch_provider.c:706, iwch_reg_user_mr() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/cxgb3/iwch_provider.c:1026, iwch_create_qp() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/amso1100/c2_provider.c:171, c2_alloc_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx4/cq.c:195, mlx4_ib_create_cq() ib_copy_from_udata() with no check on input length. - drivers/infiniband/hw/mlx4/cq.c:240, mlx4_ib_create_cq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx4/cq.c:302, mlx4_alloc_resize_umem() (called through mlx4_ib_resize_cq()) ib_copy_from_udata() with no check on input length. - drivers/infiniband/hw/mlx4/srq.c:111, mlx4_ib_create_srq() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mlx4/srq.c:193, mlx4_ib_create_srq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx4/main.c:627, mlx4_ib_alloc_ucontext() drivers/infiniband/hw/mlx4/main.c:629, mlx4_ib_alloc_ucontext() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx4/main.c:696, mlx4_ib_alloc_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx4/qp.c:677, create_qp_common() (called through mlx4_ib_create_qp()) ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/ehca/ehca_qp.c:906, internal_create_qp() (called through ehca_create_qp()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/ehca/ehca_cq.c:284, ehca_create_cq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/nes/nes_verbs.c:651, nes_alloc_ucontext() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/nes/nes_verbs.c:682, nes_alloc_ucontext() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/nes/nes_verbs.c:817, nes_alloc_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/nes/nes_verbs.c:1185, nes_create_qp() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/nes/nes_verbs.c:1392, nes_create_qp() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/nes/nes_verbs.c:1574, nes_create_cq() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/nes/nes_verbs.c:1771, nes_create_cq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/nes/nes_verbs.c:2346, nes_reg_user_mr() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/usnic/usnic_ib_verbs.c:114, usnic_ib_fill_create_qp_resp() (called through usnic_ib_create_qp()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/cxgb4/cq.c:943 c4iw_create_cq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/cxgb4/provider.c:226, c4iw_allocate_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/cxgb4/qp.c:1683, c4iw_create_qp() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mthca/mthca_provider.c:333, mthca_alloc_ucontext() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mthca/mthca_provider.c:389, mthca_alloc_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mthca/mthca_provider.c:453, mthca_create_srq() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mthca/mthca_provider.c:479, mthca_create_srq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mthca/mthca_provider.c:535, mthca_create_qp() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mthca/mthca_provider.c:658, mthca_create_cq() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mthca/mthca_provider.c:696, mthca_create_cq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mthca/mthca_provider.c:791, mthca_resize_cq() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mlx5/cq.c:526, create_cq_user() (called through mlx5_ib_create_cq()) ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mlx5/cq.c:707, mlx5_ib_create_cq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx5/srq.c:87, create_srq_user() (called through mlx5_ib_create_srq()) ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mlx5/cq.c:307, mlx5_ib_create_srq() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx5/main.c:552, mlx5_ib_alloc_ucontext() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mlx5/main.c:621, mlx5_ib_alloc_ucontext() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx5/main.c:798, mlx5_ib_alloc_pd() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx5/qp.c:500, create_qp_user() (called through create_qp_common()) ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/mlx5/qp.c:567, create_qp_user() (called through create_qp_common()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/mlx5/qp.c:752, create_qp_common() (called through mlx5_ib_create_qp()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:405, ocrdma_alloc_ucontext() ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:517, ocrdma_copy_pd_uresp() (called through ocrdma_alloc_pd()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:870, ocrdma_copy_cq_uresp() (called through ocrdma_create_cq()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:901, ocrdma_create_cq() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1109, ocrdma_copy_qp_uresp() (called through ocrdma_create_qp()) ib_copy_to_udata(), with no check on output length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1213, ocrdma_create_qp() ib_copy_from_udata(), with no check on input length. - drivers/infiniband/hw/ocrdma/ocrdma_verbs.c:1669, ocrdma_copy_srq_uresp() (called through ocrdma_create_srq()) ib_copy_to_udata(), with no check on output length. > So I think we must double check here before applying a global change > to INIT_UDATA() and/or be ready to pay the price of regression in some > vendor software (I'm not aware of) ... As you can see, they're plenty of location where having the size equal to 0 but pointer != NULL might be legal and could be used. So in the end, we must determinate where is the kernel ABI: is write(2) syscall or libibverbs ? In the first case, allowing INIT_UDATA() to set the pointer to NULL if size is 0 will break the API/ABI as it could break existing (buggy) programs (but we don't know if such exist). In the second case, it won't break anything, since libibverbs could be assumed to do the right thing (but I haven't checked it a lot). Regards. [1] [PATCH 00/22] infiniband: improve userspace input check http://marc.info/?i=cover.1376847403.git.ydroneaud@opteya.com [2] http://en.wiktionary.org/wiki/tir%C3%A9_par_les_cheveux [3] [PATCH 03/22] infiniband: ib_copy_from_udata(): check input length http://marc.info/?i=2bf102a41c51f61965ee09df827abe8fefb523a9.1376847403.git.ydroneaud@opteya.com [4] [PATCH 04/22] infiniband: ib_copy_to_udata(): check output length http://marc.info/?i=d27716a3a1c180f832d153a7402f65ea8a75b734.1376847403.git.ydroneaud@opteya.com
diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h index 9879568aed8c..d2defbbdb5da 100644 --- a/drivers/infiniband/core/uverbs.h +++ b/drivers/infiniband/core/uverbs.h @@ -47,12 +47,12 @@ #include <rdma/ib_umem.h> #include <rdma/ib_user_verbs.h> -#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \ - do { \ - (udata)->inbuf = (const void __user *) (ibuf); \ - (udata)->outbuf = (void __user *) (obuf); \ - (udata)->inlen = (ilen); \ - (udata)->outlen = (olen); \ +#define INIT_UDATA(udata, ibuf, obuf, ilen, olen) \ + do { \ + (udata)->inbuf = (ilen) ? (const void __user *) (ibuf) : NULL; \ + (udata)->outbuf = (olen) ? (void __user *) (obuf) : NULL; \ + (udata)->inlen = (ilen); \ + (udata)->outlen = (olen); \ } while (0) /* diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 34386943ebcf..76b6f217a13e 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -676,15 +676,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, return -EINVAL; } - INIT_UDATA(&ucore, - (hdr.in_words) ? buf : 0, - (unsigned long)ex_hdr.response, - hdr.in_words * 8, - hdr.out_words * 8); + INIT_UDATA(&ucore, buf, (unsigned long)ex_hdr.response, + hdr.in_words * 8, hdr.out_words * 8); INIT_UDATA(&uhw, - (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0, - (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0, + buf + ucore.inlen, + (unsigned long) ex_hdr.response + ucore.outlen, ex_hdr.provider_in_words * 8,