| Message ID | 1375741925-22179-1-git-send-email-linus.luessing@web.de |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 6 Aug 2013 00:32:05 +0200 Linus Lüssing <linus.luessing@web.de> 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 <pebolle@tiscali.nl> > Signed-off-by: Linus Lüssing <linus.luessing@web.de> > --- > net/bridge/br_multicast.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > index 61c5e81..08e576a 100644 > --- a/net/bridge/br_multicast.c > +++ b/net/bridge/br_multicast.c > @@ -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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Linus Lüssing <linus.luessing@web.de> Date: Tue, 6 Aug 2013 00:32:05 +0200 > 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 <pebolle@tiscali.nl> > Signed-off-by: Linus Lüssing <linus.luessing@web.de> Applied, thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Aug 05, 2013 at 03:42:22PM -0700, Stephen Hemminger wrote: > On Tue, 6 Aug 2013 00:32:05 +0200 > Linus Lüssing <linus.luessing@web.de> 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 <pebolle@tiscali.nl> > > Signed-off-by: Linus Lüssing <linus.luessing@web.de> > > --- > > net/bridge/br_multicast.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c > > index 61c5e81..08e576a 100644 > > --- a/net/bridge/br_multicast.c > > +++ b/net/bridge/br_multicast.c > > @@ -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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index 61c5e81..08e576a 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -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;
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 <pebolle@tiscali.nl> Signed-off-by: Linus Lüssing <linus.luessing@web.de> --- net/bridge/br_multicast.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)