Coccicheck / coccinelle catched errors on ib/hw
cover Coccicheck / coccinelle catched errors on ib/hw
Hi all,
Please find small but important fixes on InfiniBand/iWARP RDMA drivers for problems found while using coccinelle (spatch) or coccicheck.
I'm trying to patch callers of ib_copy_{from,to}_udata() to use the error code returned by functions using a semantic patch to be applied with coccinelle. The current semantic patch could be found in a git repository hosted on gitorious.org [1].
But while I'm not yet ready to submit the resulting patches to rewrite calls to ib_copy_{from,to}_udata(), I'm submitting today important fixes for errors encountered during the conversion: I've found that three callers were not setting proper error code when failing.
The third one is especially nasty as it would make (specific) application crashes on most configuration, or, if the kernel wasn't protecting itself from NULL pointer dereferences, it could allow some exploits to be successfully executed. Hopefully, /proc/sys/vm/mmap_min_addr is here to protect us. But more, it's only applicable to NetEffect iWARP driver, so I believe the vulnerability is so impracticable that it's not even worth mentioning it. People interested could find some details in the README file from a dedicated git repository along a test program used to try to trigger the NULL pointer dereference, again hosted on gitorious.og [2]. It's mostly theoretical as I haven't access to a NetEffect iWARP HCA to really exercise the test program against the iw_nes driver.
I've done a limited manual review of other infiniband/hw/ drivers with the help of another semantic patch from mine [3] (I'm a bit ashamed of it, as it's very crude and don't use all of the feature offered by coccinelle) and found no other potential kernel NULL dereference that could be triggered from uverbs layer. But you, driver maintainers, should not trust me and do your own review.
The last patches are fixes for warnings reported by coccicheck. For those who don't use it so much, coccicheck can be executed just like sparse or smatch when building the kernel using:
make C=2 CHECK=scripts/coccicheck <targets>
You will see that it's able to catch errors that the two others static analyzers are not reporting. I'm proposing fixes for the most noticeable ones.
Thanks for reviewing, testing and applying for v3.14 and stable.
Regards.
Links:
[1] https://www.gitorious.org/opteya/coccib/source/75ebf2c1033c64c1d81df13e4ae44ee99c989eba:ib_copy_udata.cocci
[2] https://www.gitorious.org/opteya/ib-hw-nes-create-qp-null
[3] https://www.gitorious.org/opteya/coccib/source/75ebf2c1033c64c1d81df13e4ae44ee99c989eba:NULL.cocci
Yann Droneaud (5):
IB/ehca: returns an error on ib_copy_to_udata() failure IB/mthca: returns an error on ib_copy_to_udata() failure IB/nes: returns an error on ib_copy_from_udata() failure instead of
NULL
IB/qib: add missing braces in do_qib_user_sdma_queue_create() IB/qib: fixup indentation in qib_ib_rcv()
drivers/infiniband/hw/ehca/ehca_cq.c | 1 + drivers/infiniband/hw/mthca/mthca_provider.c | 1 + drivers/infiniband/hw/nes/nes_verbs.c | 2 +- drivers/infiniband/hw/qib/qib_file_ops.c | 3 ++- drivers/infiniband/hw/qib/qib_verbs.c | 4 ++-- 5 files changed, 7 insertions(+), 4 deletions(-)
Signature · 1 line
1.8.5.3
PATCH 1/5 IB/ehca: returns an error on ib_copy_to_udata() failure
In case of error when writing to userspace, function ehca_create_cq() do not set an error code before following its error path.
This patch set error code to -EFAULT when ib_copy_to_udata() fail.
This was caught when using spatch (aka. coccinelle) to rewrite call to ib_copy_{from,to}_udata().
--- drivers/infiniband/hw/ehca/ehca_cq.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/hw/ehca/ehca_cq.c +1 -0
@@ -283,6 +283,7 @@ struct ib_cq *ehca_create_cq(struct ib_device *device, int cqe, int comp_vector, (my_cq->galpas.user.fw_handle & (PAGE_SIZE -1 ));if (ib_copy_to_udata (udata, &resp,sizeof (resp))) {ehca_err (device,"Copy to udata failed." ); + cq =ERR_PTR (-EFAULT ); goto create_cq_exit4; } }
Signature · 1 line
1.8.5.3
PATCH 2/5 IB/mthca: returns an error on ib_copy_to_udata() failure
In reply to Yann DroneaudIn case of error when writing to userspace, function mthca_create_cq() do not set an error code before following its error path.
This patch set error code to -EFAULT when ib_copy_to_udata() fail.
This was caught when using spatch (aka. coccinelle) to rewrite call to ib_copy_{from,to}_udata().
--- drivers/infiniband/hw/mthca/mthca_provider.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/hw/mthca/mthca_provider.c +1 -0
@@ -695,6 +695,7 @@ static struct ib_cq *mthca_create_cq(struct ib_device *ibdev, int entries,if (context &&ib_copy_to_udata (udata, &cq->cqn,sizeof (__u32))) {mthca_free_cq (to_mdev (ibdev), cq); + err = -EFAULT ; goto err_free; }
Signature · 1 line
1.8.5.3
PATCH 3/5 IB/nes: returns an error on ib_copy_from_udata() failure instead of NULL
In reply to Yann DroneaudIn case of error while accessing to userspace memory, function nes_create_qp() returns NULL instead of an error code wrapped through ERR_PTR().
But NULL is not expected by ib_uverbs_create_qp(), as it check for error with IS_ERR().
As page 0 is likely not mapped, it is going to trigger an Oops when the kernel will try to dereference NULL pointer to access to struct ib_qp's fields.
In some rare cases, page 0 could be mapped by userspace, which could turn this bug to a vulnerability that could be
exploited: the function pointers in struct ib_device will be under userspace total control.
This was caught when using spatch (aka. coccinelle) to rewrite calls to ib_copy_{from,to}_udata().
--- drivers/infiniband/hw/nes/nes_verbs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/hw/nes/nes_verbs.c +1 -1
@@ -1186,7 +1186,7 @@ static struct ib_qp *nes_create_qp(struct ib_pd *ibpd,nes_free_resource (nesadapter, nesadapter->allocated_qps, qp_num);kfree (nesqp->allocated_buffer);nes_debug (NES_DBG_QP ,"ib_copy_from_udata() Failed \n" ); -return NULL ; +return ERR_PTR (-EFAULT ); }if (req.user_wqe_buffers) { virt_wqs =1 ;
Signature · 1 line
1.8.5.3
PATCH 4/5 IB/qib: add missing braces in do_qib_user_sdma_queue_create()
In reply to Yann DroneaudCommit c804f07248895ff9c moved qib_assign_ctxt() to do_qib_user_sdma_queue_create() but dropped the braces around the statements.
This was spotted by coccicheck (coccinelle/spatch):
$ make C=2 CHECK=scripts/coccicheck drivers/infiniband/hw/qib/
CHECK drivers/infiniband/hw/qib/qib_file_ops.c
drivers/infiniband/hw/qib/qib_file_ops.c:1583:2-23: code aligned with following code on line 1587
This patch adds braces back.
--- drivers/infiniband/hw/qib/qib_file_ops.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/infiniband/hw/qib/qib_file_ops.c +2 -1
@@ -1578,7 +1578,7 @@ static int do_qib_user_sdma_queue_create(struct file *fp)struct qib_ctxtdata *rcd = fd->rcd;struct qib_devdata *dd = rcd->dd; -if (dd->flags &QIB_HAS_SEND_DMA ) +if (dd->flags &QIB_HAS_SEND_DMA ) { fd->pq = qib_user_sdma_queue_create(&dd->pcidev->dev, dd->unit,
@@ -1586,6 +1586,7 @@ static int do_qib_user_sdma_queue_create(struct file *fp) fd->subctxt);if (!fd->pq)return -ENOMEM ; + }return 0 ; }
Signature · 1 line
1.8.5.3
PATCH 4/5 IB/qib: add missing braces in do_qib_user_sdma_queue_create()
Quoted 4 lines
Subject: [PATCH for v3.14 4/5] IB/qib: add missing braces in
do_qib_user_sdma_queue_create()Signed-off-by Yann Droneaud <[email protected]>
Thanks for the patch!
PATCH 5/5 IB/qib: fixup indentation in qib_ib_rcv()
In reply to Yann DroneaudCommit af061a644a0e4d4778 add some code in qib_ib_rcv() which trigger a warning from coccicheck (coccinelle/spatch):
$ make C=2 CHECK=scripts/coccicheck drivers/infiniband/hw/qib/
CHECK drivers/infiniband/hw/qib/qib_verbs.c
drivers/infiniband/hw/qib/qib_verbs.c:679:5-32: code aligned with following code on line 681
CC [M] drivers/infiniband/hw/qib/qib_verbs.o
In fact, according to similar code in qib_kreceive(), qib_ib_rcv() code is correct but improperly indented.
This patch fix indentation for the misaligned portion.
--- drivers/infiniband/hw/qib/qib_verbs.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/hw/qib/qib_verbs.c +2 -2
@@ -678,8 +678,8 @@ void qib_ib_rcv(struct qib_ctxtdata *rcd, void *rhdr, void *data, u32 tlen) &rcd->lookaside_qp->refcount))wake_up ( &rcd->lookaside_qp->wait); - rcd->lookaside_qp =NULL ; - } + rcd->lookaside_qp =NULL ; + } }if (!rcd->lookaside_qp) { qp =qib_lookup_qpn (ibp, qp_num);
Signature · 1 line
1.8.5.3
PATCH 5/5 IB/qib: fixup indentation in qib_ib_rcv()
Quoted 3 lines
Subject: [PATCH for v3.14 5/5] IB/qib: fixup indentation in
qib_ib_rcv()Signed-off-by Yann Droneaud <[email protected]>
Thanks for the patch!
A message that is not in the archive (sent off-list or not yet received): cover.1394485254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Hi Roland,
Quoted 3 lines · Yann Droneaud
Please find small but important fixes on InfiniBand/iWARP RDMA drivers for problems found while using coccinelle (spatch) or coccicheck.
Thanks for applying those patches in your for-next branch.
You haven't made any comment on the patches but I've found you have redacted a bit my commit messages. In particular, it seems that somes "Cc:" were removed. I'm primary concerned about removal of Cc:
[email protected] on one patch.
3 patches were cleared from some Cc:
- [PATCH for v3.14 1/5] IB/ehca: returns an error on ib_copy_to_udata() failure
The patch as I proposed it:
The patch in your tree:
Following Cc: were removed:
- [PATCH for v3.14 2/5] IB/mthca: returns an error on ib_copy_to_udata() failure
Following CC: were removed
Here removing [email protected] doesn't sound right.
- [PATCH for v3.14 3/5] IB/nes: returns an error on ib_copy_from_udata() failure instead of NULL
Following Cc: were removed:
Could you help me to understand the reason why your selectively remove some 'Cc:' on some patches while leaving them on other patches.
BTW, I would like to be able to help you by providing 'correct' patches you could apply without requiring too much work from you.
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): 1395753355.2895.12.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/[email protected]
Quoted 1 line · Yann Droneaud
Here removing [email protected] doesn't sound right.
Sorry, I've added that back.
Quoted 2 lines
Could you help me to understand the reason why your selectively remove some 'Cc:' on some patches while leaving them on other patches.
Typically I don't see much point in putting a list of email addresses of people who might (or might not) have some connection to the patch into the changelog. "Cc:" means "I tried to get this person's attention on the patch but they didn't reply in time" and I don't see much value in persisting that information forever, so I strip it when I catch it. The IB/RDMA community has a lot of people for whom English is not their native language, so I'm pretty much in the habit of trying to polish all changelogs as they go by. "Cc: stable" is of course the one exception to my stripping.
- R.
--
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