Skip to contentSkip to conversation

bridge: don't try to update timers in case of broken MLD queries

PATCH Applied Linus Lüssing 3 participants 4 messages Aug 5, 2013 – Aug 6, 2013

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore

Linus Lüssing

author
#0 lore raw

PATCH 1/1 bridge: don't try to update timers in case of broken MLD queries

Currently we are reading an uninitialized value for the max_delay variable when snooping an MLD query message of invalid length and would update our timers with that.

Fixing this by simply ignoring such broken MLD queries (just like we do for IGMP already).

This is a regression introduced by:
"bridge: disable snooping if there is no querier" (b00589af3b04)

Reported-by Paul Bolle <[email protected]>
Signed-off-by Linus Lüssing <[email protected]>
---
 net/bridge/br_multicast.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/bridge/br_multicast.c +1 -1
index 61c5e81..08e576a 100644
@@ -1195,7 +1195,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
 		if (max_delay)
 			group = &mld->mld_mca;
-	} else if (skb->len >= sizeof(*mld2q)) {
+	} else {
 		if (!pskb_may_pull(skb, sizeof(*mld2q))) {
 			err = -EINVAL;
 			goto out;
Signature · 1 line
1.7.10.4
#1 lore raw
Quoted 21 lines · On Tue, 6 Aug 2013 00:32:05 +0200 Linus Lüssing <[email protected]> wrote:

Currently we are reading an uninitialized value for the max_delay variable when snooping an MLD query message of invalid length and would update our timers with that.

Fixing this by simply ignoring such broken MLD queries (just like we do for IGMP already).

This is a regression introduced by:
"bridge: disable snooping if there is no querier" (b00589af3b04)

Reported-by Paul Bolle <[email protected]>
Signed-off-by Linus Lüssing <[email protected]>
---
 net/bridge/br_multicast.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
net/bridge/br_multicast.c +0 -0
index 61c5e81..08e576a 100644

net/bridge/br_multicast.c static int br_ip6_multicast_query(struct net_bridge *br, · line 1201

Quoted 8 lines
net/bridge/br_multicast.c +1 -1
@@ -1195,7 +1195,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
 		if (max_delay)
 			group = &mld->mld_mca;
-	} else if (skb->len >= sizeof(*mld2q)) {
+	} else {
 		if (!pskb_may_pull(skb, sizeof(*mld2q))) {
 			err = -EINVAL;
 			goto out;

Why not use else if here, other than that looks great.

author
#2 lore raw
Quoted 19 lines · Stephen Hemminger
Quoted 16 lines · On Tue, 6 Aug 2013 00:32:05 +0200 Linus Lüssing <[email protected]> wrote:

Currently we are reading an uninitialized value for the max_delay variable when snooping an MLD query message of invalid length and would update our timers with that.

Fixing this by simply ignoring such broken MLD queries (just like we do for IGMP already).

This is a regression introduced by:
"bridge: disable snooping if there is no querier" (b00589af3b04)

Reported-by Paul Bolle <[email protected]>
Signed-off-by Linus Lüssing <[email protected]>
---
 net/bridge/br_multicast.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 15 lines

net/bridge/br_multicast.c static int br_ip6_multicast_query(struct net_bridge *br, · line 1201

Quoted 13 lines
net/bridge/br_multicast.c +1 -1
index 61c5e81..08e576a 100644
@@ -1195,7 +1195,7 @@ static int br_ip6_multicast_query(struct net_bridge *br,
 		max_delay = msecs_to_jiffies(ntohs(mld->mld_maxdelay));
 		if (max_delay)
 			group = &mld->mld_mca;
-	} else if (skb->len >= sizeof(*mld2q)) {
+	} else {
 		if (!pskb_may_pull(skb, sizeof(*mld2q))) {
 			err = -EINVAL;
 			goto out;

Why not use else if here, other than that looks great.

Because it isn't really necessary, it is basically included in the pskb_may_pull() already, just like it is in the according IGMP code path.

And I thought it'd be nicer to handle it the same way as in the IGMP code path to avoid diverging too much.

Cheers, Linus

maintainer Applied
#3 lore raw
In reply to Linus Lüssing

From: Linus Lüssing <[email protected]>
Date: Tue, 6 Aug 2013 00:32:05 +0200

Quoted 4 lines

Currently we are reading an uninitialized value for the max_delay variable when snooping an MLD query message of invalid length and would update our timers with that.

Quoted 8 lines

Fixing this by simply ignoring such broken MLD queries (just like we do for IGMP already).

This is a regression introduced by:
"bridge: disable snooping if there is no querier" (b00589af3b04)

Reported-by Paul Bolle <[email protected]>
Signed-off-by Linus Lüssing <[email protected]>

Applied, thanks.