[openib-general] Re: [PATCH] check for valid MGID in user space

[Jack Morgenstein]

I was operating under the assumption that kernel code is trusted code (so as not to burden
the kernel will too many tests of correctness.

Below is a patch for verbs.c, which will accomplish the same check for both kernel
user space applications.  In addition, since the fix is on the same line, the patch
also checks that the QP we are trying to attach/detach is UD only (per IB Spec 11.3.1).

Signed-off-by: Jack Morgenstein <jackm at mellanox.co.il>

Index: linux-kernel/infiniband/core/verbs.c
===================================================================
--- linux-kernel/infiniband/core/verbs.c	(revision 3532)
+++ linux-kernel/infiniband/core/verbs.c	(working copy)
@@ -523,16 +523,16 @@
 
 int ib_attach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid)
 {
-	return qp->device->attach_mcast ?
-		qp->device->attach_mcast(qp, gid, lid) :
-		-ENOSYS;
+	return !qp->device->attach_mcast ? -ENOSYS :
+		(gid->raw[0] != 0xFF || qp->qp_type != IB_QPT_UD) ? -EINVAL :
+		qp->device->attach_mcast(qp, gid, lid);
 }
 EXPORT_SYMBOL(ib_attach_mcast);
 
 int ib_detach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid)
 {
-	return qp->device->detach_mcast ?
-		qp->device->detach_mcast(qp, gid, lid) :
-		-ENOSYS;
+	return !qp->device->detach_mcast ? -ENOSYS :
+		(gid->raw[0] != 0xFF || qp->qp_type != IB_QPT_UD) ? -EINVAL :
+		qp->device->detach_mcast(qp, gid, lid);
 }
 EXPORT_SYMBOL(ib_detach_mcast);


On Sun, Sep 25, 2005 at 12:10:38AM +0300, Roland Dreier wrote:
>     Jack> The following patch checks validity of MGID when
>     Jack> attaching/detaching a QP to/from a multicast group (for
>     Jack> user-space only). IB spec demands that multicast gids start
>     Jack> with 0xFF in 0-th byte. (IB Spec v1.2,section 4.1.1 (page
>     Jack> 144)).
> 
> This seems like a strange place to check the condition.  Why do we
> want to allow kernel callers to use invalid MGIDs?  In other words it
> would seem more appropriate to me to put the check in verbs.c.
> 
> Also no Signed-off-by: line for your patch...
> 
>  - R.