[ofa-general] [PATCH] IPoIB: check multicast address format (V2)

[Jason Gunthorpe]

Check that the format of the multicast link address is correct before
taking it from dev->mc_list to priv->multicast_list. This way we never
try to send a bogus address to the SA, and prevents badness from
erronous 'ip maddr addr add', broken bonding drivers, or whatever.

Signed-off-by: Jason Gunthorpe <jgunthorpe at obsidianresearch.com>
---
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

> The idea seems sound but checkpatch.pl gives 6 errors for this small
> patch!  Also:

Indeed, sorry, I don't do this very often, forgot that step. I added
the 6 missing spaces.

> name of the function could make it clearer what the expected return
> value is ... eg mcast_addr_is_valid() or something like that.

Yes, done
 
>  > +	if (addrlen != 20) 
> We have INFINIBAND_ALEN defined, seems better than a magic # here.

Great, I looked for that for a few mins..

>  > +	if (memcmp(addr,broadcast,6) != 0)
 
> Personal taste here, but "if (foo != 0)" always seems silly to me when
> we could just do "if (foo)" -- haven't looked at what usage of memcmp()
> is more idiomatic in the kernel tho.

OK, much of the rest of the kernel is without the operator. I do it
this way because the discordance of:
   if (cmp(a, b)) means a is not b, 
   if (!cmp(a, b)) means a is b
   if (is_something(a)) means a is something
hurts my brain, even though I know that is how it all
works..

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 425e311..a6485c4 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -758,6 +758,20 @@ void ipoib_mcast_dev_flush(struct net_device *dev)
 	}
 }
 
+static int ipoib_mcast_addr_is_valid(const u8 *addr, unsigned int addrlen,
+				     const u8 *broadcast)
+{
+	if (addrlen != INFINIBAND_ALEN)
+		return 0;
+	/* reserved QPN, prefix, scope */
+	if (memcmp(addr, broadcast, 6))
+		return 0;
+	/* signature lower, pkey */
+	if (memcmp(addr + 7, broadcast + 7, 3))
+		return 0;
+	return 1;
+}
+
 void ipoib_mcast_restart_task(struct work_struct *work)
 {
 	struct ipoib_dev_priv *priv =
@@ -791,6 +805,11 @@ void ipoib_mcast_restart_task(struct work_struct *work)
 	for (mclist = dev->mc_list; mclist; mclist = mclist->next) {
 		union ib_gid mgid;
 
+		if (!ipoib_mcast_addr_is_valid(mclist->dmi_addr,
+					       mclist->dmi_addrlen,
+					       dev->broadcast))
+			continue;
+
 		memcpy(mgid.raw, mclist->dmi_addr + 4, sizeof mgid);
 
 		mcast = __ipoib_mcast_find(dev, &mgid);
-- 
1.5.4.2