[PATCH for v3.14 0/5] Coccicheck / coccinelle catched errors on ib/hw

[PATCH for v3.14 0/5] Coccicheck / coccinelle catched errors on ib/hw

[Yann Droneaud]

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(-)

-- 
1.8.5.3

[PATCH for v3.14 1/5] IB/ehca: returns an error on ib_copy_to_udata() failure

[Yann Droneaud]

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().

Link: https://www.gitorious.org/opteya/coccib/source/75ebf2c1033c64c1d81df13e4ae44ee99c989eba:ib_copy_udata.cocci
Link: http://marc.info/?i=cover.1394485254.git.ydroneaud@opteya.com
Cc: Hoang-Nam Nguyen <hnguyen@de.ibm.com>
Cc: Christoph Raisch <raisch@de.ibm.com>
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: cocci@systeme.lip6.fr
Cc: stable@vger.kernel.org
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 drivers/infiniband/hw/ehca/ehca_cq.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/hw/ehca/ehca_cq.c b/drivers/infiniband/hw/ehca/ehca_cq.c
index 212150c25ea0..8cc837537768 100644
--- a/drivers/infiniband/hw/ehca/ehca_cq.c
+++ b/drivers/infiniband/hw/ehca/ehca_cq.c
@@ -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;
 		}
 	}
-- 
1.8.5.3

[PATCH for v3.14 2/5] IB/mthca: returns an error on ib_copy_to_udata() failure

[Yann Droneaud]

In 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().

Link: https://www.gitorious.org/opteya/coccib/source/75ebf2c1033c64c1d81df13e4ae44ee99c989eba:ib_copy_udata.cocci
Link: http://marc.info/?i=cover.1394485254.git.ydroneaud@opteya.com
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: cocci@systeme.lip6.fr
Cc: stable@vger.kernel.org
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 drivers/infiniband/hw/mthca/mthca_provider.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/hw/mthca/mthca_provider.c b/drivers/infiniband/hw/mthca/mthca_provider.c
index 5b71d43bd89c..42dde06fdb91 100644
--- a/drivers/infiniband/hw/mthca/mthca_provider.c
+++ b/drivers/infiniband/hw/mthca/mthca_provider.c
@@ -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;
 	}
 
-- 
1.8.5.3

[PATCH for v3.14 3/5] IB/nes: returns an error on ib_copy_from_udata() failure instead of NULL

[Yann Droneaud]

In 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().

Link: https://www.gitorious.org/opteya/ib-hw-nes-create-qp-null
Link: https://www.gitorious.org/opteya/coccib/source/75ebf2c1033c64c1d81df13e4ae44ee99c989eba:ib_copy_udata.cocci
Link: http://marc.info/?i=cover.1394485254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: Faisal Latif <faisal.latif-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Tatyana E. Nikolova <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Julia Lawall <julia.lawall-L2FTfq7BK8M@public.gmane.org>
Cc: cocci-/FJkirnvOdkvYVN+rsErww@public.gmane.org
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/hw/nes/nes_verbs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/nes/nes_verbs.c b/drivers/infiniband/hw/nes/nes_verbs.c
index 8308e3634767..eb624611f94b 100644
--- a/drivers/infiniband/hw/nes/nes_verbs.c
+++ b/drivers/infiniband/hw/nes/nes_verbs.c
@@ -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;
-- 
1.8.5.3

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for v3.14 0/5] Coccicheck / coccinelle catched errors on ib/hw

[Yann Droneaud]

Hi Roland,

Le lundi 10 mars 2014 à 23:06 +0100, Yann Droneaud a écrit :

> 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:
stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org 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:

http://marc.info/?i=a74f09417f54a40f12475b65b29094173424ff8d.1394485254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

The patch in your tree:

https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=dea2494d6901d376b8d10d297de23021fbfc3be7

Following Cc: were removed:

Cc: Hoang-Nam Nguyen <hnguyen-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
Cc: Christoph Raisch <raisch-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>
Cc: Julia Lawall <julia.lawall-L2FTfq7BK8M@public.gmane.org>
Cc: cocci-/FJkirnvOdkvYVN+rsErww@public.gmane.org

- [PATCH for v3.14 2/5] IB/mthca: returns an error on ib_copy_to_udata()
failure

http://marc.info/?i=014a0e999228bcbefd32667b341a0fc91a8cd3c9.1394485254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=13ef5c07e649b49519e7dec5896c8772a6eb5a76

Following CC: were removed

Cc: Julia Lawall <julia.lawall-L2FTfq7BK8M@public.gmane.org>
Cc: cocci-/FJkirnvOdkvYVN+rsErww@public.gmane.org
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Here removing stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org doesn't sound right.

- [PATCH for v3.14 3/5] IB/nes: returns an error on ib_copy_from_udata()
failure instead of NULL

http://marc.info/?i=69b8f22f0d52b1c1f97b0af2b5bcecbe14cb1da2.1394485254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

https://git.kernel.org/cgit/linux/kernel/git/roland/infiniband.git/commit/?h=for-next&id=9d194d1025f463392feafa26ff8c2d8247f71be1

Following Cc: were removed:

Cc: Faisal Latif <faisal.latif-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Tatyana E. Nikolova <tatyana.e.nikolova-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Cc: Julia Lawall <julia.lawall-L2FTfq7BK8M@public.gmane.org>
Cc: cocci-/FJkirnvOdkvYVN+rsErww@public.gmane.org

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.

-- 
Yann Droneaud
OPTEYA


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for v3.14 0/5] Coccicheck / coccinelle catched errors on ib/hw

[Roland Dreier]

On Tue, Mar 25, 2014 at 6:15 AM, Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org> wrote:
> Here removing stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org doesn't sound right.

Sorry, I've added that back.

> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for v3.14 4/5] IB/qib: add missing braces in do_qib_user_sdma_queue_create()

[Yann Droneaud]

Commit 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.

Link: http://marc.info/?i=cover.1394485254.git.ydroneaud@opteya.com
Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
Cc: infinipath@intel.com
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: cocci@systeme.lip6.fr
Cc: stable@vger.kernel.org
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 drivers/infiniband/hw/qib/qib_file_ops.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/qib/qib_file_ops.c b/drivers/infiniband/hw/qib/qib_file_ops.c
index 275f247f9fca..2023cd61b897 100644
--- a/drivers/infiniband/hw/qib/qib_file_ops.c
+++ b/drivers/infiniband/hw/qib/qib_file_ops.c
@@ -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;
 }
-- 
1.8.5.3

RE: [PATCH for v3.14 4/5] IB/qib: add missing braces in do_qib_user_sdma_queue_create()

[Marciniszyn, Mike]

> Subject: [PATCH for v3.14 4/5] IB/qib: add missing braces in
> do_qib_user_sdma_queue_create()
> 
> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>

Thanks for the patch!

Tested-by: Mike Marciniszyn <mike.marciniszyn@intel.com>
Acked-by: Mike Marciniszyn <mike.marciniszyn@intel.com>

[PATCH for v3.14 5/5] IB/qib: fixup indentation in qib_ib_rcv()

[Yann Droneaud]

Commit 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.

Link: http://marc.info/?i=cover.1394485254.git.ydroneaud@opteya.com
Cc: Mike Marciniszyn <mike.marciniszyn@intel.com>
Cc: infinipath@intel.com
Cc: Julia Lawall <julia.lawall@lip6.fr>
Cc: cocci@systeme.lip6.fr
Signed-off-by: Yann Droneaud <ydroneaud@opteya.com>
---
 drivers/infiniband/hw/qib/qib_verbs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/qib/qib_verbs.c b/drivers/infiniband/hw/qib/qib_verbs.c
index 092b0bb1bb78..b53e0a2dc545 100644
--- a/drivers/infiniband/hw/qib/qib_verbs.c
+++ b/drivers/infiniband/hw/qib/qib_verbs.c
@@ -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);
-- 
1.8.5.3

RE: [PATCH for v3.14 5/5] IB/qib: fixup indentation in qib_ib_rcv()

[Marciniszyn, Mike]

> Subject: [PATCH for v3.14 5/5] IB/qib: fixup indentation in qib_ib_rcv()
> 
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>

Thanks for the patch!

Tested-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Acked-by: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html