Skip to contentSkip to conversation

IPv6: Check packet size when process Multicast Address and Source Specific Query

PATCH In review Yan Zheng 4 participants 6 messages Oct 31, 2005 – Nov 1, 2005

Subsystems (3)
Networking Network drivers IPv4/IPv6
lore
Share & tools
lore

Yan Zheng

author
#0 lore raw

PATCH 1/1 IPv6: Check packet size when process Multicast Address and Source Specific Query

Signed-off-by Yan Zheng <[email protected]>
net/ipv6/mcast.c +0 -0
Index: net/ipv6/mcast.c
================================================================================
@@ -1156,7 +1156,12 @@ int igmp6_event_query(struct sk_buff *sk

			return 0;
		}
		/* mark sources to include, if group & source-specific */

- mark = mlh2->nsrcs != 0;
+ if (mlh2->nsrcs != 0) {
+ if (!pskb_may_pull(skb, mlh2->nsrcs * sizeof(struct in6_addr) +
+ (sizeof(struct mld2_query) - sizeof(struct icmp6hdr))))
+ return -EINVAL;
+ mark = 1;
+ }

	} else {
		in6_dev_put(idev);
		return -EINVAL;
#1 lore raw

In article <[email protected]> (at Mon, 31 Oct 2005 13:20:21 +0800), Yan Zheng <[email protected]> says:

Quoted 11 lines
Signed-off-by Yan Zheng <[email protected]>

Index: net/ipv6/mcast.c
================================================================================

net/ipv6/mcast.c +0 -0
@@ -1156,7 +1156,12 @@ int igmp6_event_query(struct sk_buff *sk
 			return 0;
 		}
 		/* mark sources to include, if group & source-specific */

net/ipv6/mcast.c int igmp6_event_query(struct sk_buff *sk · line 1167

Quoted 10 lines
net/ipv6/mcast.c +6 -1
-		mark = mlh2->nsrcs != 0;
+		if (mlh2->nsrcs != 0) {
+			if (!pskb_may_pull(skb, mlh2->nsrcs * sizeof(struct in6_addr) +
+				(sizeof(struct mld2_query) - sizeof(struct icmp6hdr))))
+				return -EINVAL;
+			mark = 1;
+		}
 	} else {
 		in6_dev_put(idev);
 		return -EINVAL;

You cannot continue using mlh2, local copy of skb->h.raw after pskb_may_pull(). Please refresh it.

--yoshfuji

author
#2 lore raw
Quoted 6 lines

You cannot continue using mlh2, local copy of skb->h.raw after pskb_may_pull(). Please refresh it.

--yoshfuji

My mistake. sorry.
I hope the new one is correct.

Regards
================================================================================

net/ipv6/mcast.c +0 -0
@@ -1087,7 +1087,7 @@ static void mld_marksources(struct ifmca

int igmp6_event_query(struct sk_buff *skb)
{
- struct mld2_query *mlh2 = (struct mld2_query *) skb->h.raw;

+	struct mld2_query *mlh2 = NULL;
	struct ifmcaddr6 *ma;
	struct in6_addr *group;
	unsigned long max_delay;
@@ -1140,6 +1140,13 @@ int igmp6_event_query(struct sk_buff *sk
		/* clear deleted report items */
		mld_clear_delrec(idev);
	} else if (len >= 28) {

+ int srcs_offset = sizeof(struct mld2_query) - + sizeof(struct icmp6hdr);
+ if (!pskb_may_pull(skb, srcs_offset)) {
+ in6_dev_put(idev);
+ return -EINVAL;
+ }

+		mlh2 = (struct mld2_query *) skb->h.raw;
		max_delay = (MLDV2_MRC(ntohs(mlh2->mrc))*HZ)/1000;
		if (!max_delay)
			max_delay = 1;
@@ -1156,7 +1163,15 @@ int igmp6_event_query(struct sk_buff *sk
			return 0;
		}
		/* mark sources to include, if group & source-specific */

- mark = mlh2->nsrcs != 0;
+ if (mlh2->nsrcs != 0) {
+ if (!pskb_may_pull(skb, srcs_offset + + mlh2->nsrcs * sizeof(struct in6_addr))) {
+ in6_dev_put(idev);
+ return -EINVAL;
+ }
+ mlh2 = (struct mld2_query *) skb->h.raw;
+ mark = 1;
+ }

	} else {
		in6_dev_put(idev);
		return -EINVAL;
#4 lore raw
In reply to Yan Zheng

I think this should be modelled after the equivalent code in IGMPv3. See igmp_heard_query() in net/ipv4/igmp.c. For ease of maintenance, the code should be structured exactly the same way, except for necessary differences, of course.

I haven't seen enough context yet, but I think you need to check for the query header itself, too (as done in IGMPv3).

I'm reviewing your other patches as well.

                                        +-DLS
#5 lore raw
Quoted 11 lines

I think this should be modelled after the equivalent code in IGMPv3. See igmp_heard_query() in net/ipv4/igmp.c. For ease of maintenance, the code should be structured exactly the same way, except for necessary differences, of course.

I haven't seen enough context yet, but I think you need to check for the query header itself, too (as done in IGMPv3).

I'm reviewing your other patches as well.

                                        +-DLS

Yes . It's better to drop invalid query earlier.