net: ipmr/ip6mr: prevent out-of-bounds vif_table access

PATCH Applied Nicolas Dichtel 2 participants 5 messages Mar 26, 2010 – Mar 27, 2010

Subsystems (3)

Networking Network drivers IPv4/IPv6

lore

Share & tools

lore

Nicolas Dichtel

author

2010-03-26 09:45 UTC #0 lore raw

PATCH 1/1 net: ipmr/ip6mr: prevent out-of-bounds vif_table access

Hi,

please consider the attached patch about IPv4 and IPv6 multicast.

Regards,
Nicolas

Quoted 1 line

From fb0b874d55bfe2b81499d4c17598b94ac6c58594 Mon Sep 17 00:00:00 2001

From: Nicolas Dichtel <[email protected]>
Date: Fri, 26 Mar 2010 10:12:34 +0100
Subject: [PATCH] net: ipmr/ip6mr: prevent out-of-bounds vif_table access

When cache is unresolved, c->mf[6]c_parent is set to 65535 and minvif, maxvif are not initialized, hence we must avoid to parse IIF and OIF.
A second problem can happen when the user dumps a cache entry where a VIF, that was referenced at creation time, has been removed.

Signed-off-by Nicolas Dichtel <[email protected]>

---
 net/ipv4/ipmr.c  |   11 +++++++----
 net/ipv6/ip6mr.c |   11 +++++++----
 2 files changed, 14 insertions(+), 8 deletions(-)

net/ipv4/ipmr.c +7 -4

index 0b9d03c..d0a6092 100644
@@ -1616,17 +1616,20 @@ ipmr_fill_mroute(struct sk_buff *skb, struct mfc_cache *c, struct rtmsg *rtm)
 	int ct;
 	struct rtnexthop *nhp;
 	struct net *net = mfc_net(c);
-	struct net_device *dev = net->ipv4.vif_table[c->mfc_parent].dev;
 	u8 *b = skb_tail_pointer(skb);
 	struct rtattr *mp_head;
 
-	if (dev)
-		RTA_PUT(skb, RTA_IIF, 4, &dev->ifindex);
+	/* If cache is unresolved, don't try to parse IIF and OIF */
+	if (c->mfc_parent > MAXVIFS)
+		return -ENOENT;
+
+	if (VIF_EXISTS(net, c->mfc_parent))
+		RTA_PUT(skb, RTA_IIF, 4, &net->ipv4.vif_table[c->mfc_parent].dev->ifindex);
 
 	mp_head = (struct rtattr *)skb_put(skb, RTA_LENGTH(0));
 
 	for (ct = c->mfc_un.res.minvif; ct < c->mfc_un.res.maxvif; ct++) {
-		if (c->mfc_un.res.ttls[ct] < 255) {
+		if (VIF_EXISTS(net, ct) && c->mfc_un.res.ttls[ct] < 255) {
 			if (skb_tailroom(skb) < RTA_ALIGN(RTA_ALIGN(sizeof(*nhp)) + 4))
 				goto rtattr_failure;
 			nhp = (struct rtnexthop *)skb_put(skb, RTA_ALIGN(sizeof(*nhp)));

net/ipv6/ip6mr.c +7 -4

index 23e4ac0..27acfb5 100644
@@ -1695,17 +1695,20 @@ ip6mr_fill_mroute(struct sk_buff *skb, struct mfc6_cache *c, struct rtmsg *rtm)
 	int ct;
 	struct rtnexthop *nhp;
 	struct net *net = mfc6_net(c);
-	struct net_device *dev = net->ipv6.vif6_table[c->mf6c_parent].dev;
 	u8 *b = skb_tail_pointer(skb);
 	struct rtattr *mp_head;
 
-	if (dev)
-		RTA_PUT(skb, RTA_IIF, 4, &dev->ifindex);
+	/* If cache is unresolved, don't try to parse IIF and OIF */
+	if (c->mf6c_parent > MAXMIFS)
+		return -ENOENT;
+
+	if (MIF_EXISTS(net, c->mf6c_parent))
+		RTA_PUT(skb, RTA_IIF, 4, &net->ipv6.vif6_table[c->mf6c_parent].dev->ifindex);
 
 	mp_head = (struct rtattr *)skb_put(skb, RTA_LENGTH(0));
 
 	for (ct = c->mfc_un.res.minvif; ct < c->mfc_un.res.maxvif; ct++) {
-		if (c->mfc_un.res.ttls[ct] < 255) {
+		if (MIF_EXISTS(net, ct) && c->mfc_un.res.ttls[ct] < 255) {
 			if (skb_tailroom(skb) < RTA_ALIGN(RTA_ALIGN(sizeof(*nhp)) + 4))
 				goto rtattr_failure;
 			nhp = (struct rtnexthop *)skb_put(skb, RTA_ALIGN(sizeof(*nhp)));

Signature · 1 line

1.5.4.5

David Miller

maintainer

2010-03-26 16:51 UTC #1 lore raw

From: Nicolas Dichtel <[email protected]>
Date: Fri, 26 Mar 2010 10:45:35 +0100

Quoted 1 line

please consider the attached patch about IPv4 and IPv6 multicast.

Already fixed by Patrick McHardy recently:

commit a50436f2cd6e85794f7e1aad795ca8302177b896

Author: Patrick McHardy <[email protected]>
Date:   Wed Mar 17 06:04:14 2010 +0000

net: ipmr/ip6mr: fix potential out-of-bounds vif_table access

mfc_parent of cache entries is used to index into the vif_table and is initialised from mfcctl->mfcc_parent. This can take values of to 2^16-1, while the vif_table has only MAXVIFS (32) entries. The same problem affects ip6mr.

Refuse invalid values to fix a potential out-of-bounds access. Unlike the other validity checks, this is checked in ipmr_mfc_add() instead of the setsockopt handler since its unused in the delete path and might be uninitialized.

Signed-off-by Patrick McHardy <[email protected]>

Signed-off-by David S. Miller <[email protected]>

net/ipv4/ipmr.c +3 -0

index 8582e12..0b9d03c 100644
@@ -802,6 +802,9 @@ static int ipmr_mfc_add(struct net *net, struct mfcctl *mfc, int mrtsock)
 	int line;
 	struct mfc_cache *uc, *c, **cp;
 
+	if (mfc->mfcc_parent >= MAXVIFS)
+		return -ENFILE;
+
 	line = MFC_HASH(mfc->mfcc_mcastgrp.s_addr, mfc->mfcc_origin.s_addr);
 
 	for (cp = &net->ipv4.mfc_cache_array[line];

net/ipv6/ip6mr.c +3 -0

index 52e0f74..23e4ac0 100644
@@ -1113,6 +1113,9 @@ static int ip6mr_mfc_add(struct net *net, struct mf6cctl *mfc, int mrtsock)
 	unsigned char ttls[MAXMIFS];
 	int i;
 
+	if (mfc->mf6cc_parent >= MAXMIFS)
+		return -ENFILE;
+
 	memset(ttls, 255, MAXMIFS);
 	for (i = 0; i < MAXMIFS; i++) {
 		if (IF_ISSET(i, &mfc->mf6cc_ifset))

Nicolas Dichtel

author

2010-03-26 17:19 UTC #2 lore raw

yes, but I think that it is not enough, ip[6]mr_mfc_add() is only called via setsockopt().

When a multicast packet arrived in ip6_mr_input(), if there is no cache ip6mr_cache_unresolved() will be called and this function will add an entry with parent == 65535.

And the second problem is that when a vif is removed, no cleanup is made in cache entry. Hence, we can have a cache entry which points to an invalid vif (dev is set ot NULL).

Regards,
Nicolas

Quoted 54 lines · David Miller

From: Nicolas Dichtel <[email protected]>
Date: Fri, 26 Mar 2010 10:45:35 +0100
Quoted 1 line
please consider the attached patch about IPv4 and IPv6 multicast.
Already fixed by Patrick McHardy recently:
commit a50436f2cd6e85794f7e1aad795ca8302177b896
Author: Patrick McHardy <[email protected]>
Date:   Wed Mar 17 06:04:14 2010 +0000
net: ipmr/ip6mr: fix potential out-of-bounds vif_table access
mfc_parent of cache entries is used to index into the vif_table and is initialised from mfcctl->mfcc_parent. This can take values of to 2^16-1, while the vif_table has only MAXVIFS (32) entries. The same problem affects ip6mr.
Refuse invalid values to fix a potential out-of-bounds access. Unlike the other validity checks, this is checked in ipmr_mfc_add() instead of the setsockopt handler since its unused in the delete path and might be uninitialized.
Signed-off-by Patrick McHardy <[email protected]>
Signed-off-by David S. Miller <[email protected]>
net/ipv4/ipmr.c +3 -0
index 8582e12..0b9d03c 100644
@@ -802,6 +802,9 @@ static int ipmr_mfc_add(struct net *net, struct mfcctl *mfc, int mrtsock)
 	int line;
 	struct mfc_cache *uc, *c, **cp;
 
+	if (mfc->mfcc_parent >= MAXVIFS)
+		return -ENFILE;
+
 	line = MFC_HASH(mfc->mfcc_mcastgrp.s_addr, mfc->mfcc_origin.s_addr);
 
 	for (cp = &net->ipv4.mfc_cache_array[line];
net/ipv6/ip6mr.c +3 -0
index 52e0f74..23e4ac0 100644
@@ -1113,6 +1113,9 @@ static int ip6mr_mfc_add(struct net *net, struct mf6cctl *mfc, int mrtsock)
 	unsigned char ttls[MAXMIFS];
 	int i;
 
+	if (mfc->mf6cc_parent >= MAXMIFS)
+		return -ENFILE;
+
 	memset(ttls, 255, MAXMIFS);
 	for (i = 0; i < MAXMIFS; i++) {
 		if (IF_ISSET(i, &mfc->mf6cc_ifset))

David Miller

maintainer

2010-03-26 18:11 UTC #3 lore raw

From: Nicolas Dichtel <[email protected]>
Date: Fri, 26 Mar 2010 18:19:34 +0100

Quoted 10 lines

yes, but I think that it is not enough, ip[6]mr_mfc_add() is only called via setsockopt().
When a multicast packet arrived in ip6_mr_input(), if there is no cache ip6mr_cache_unresolved() will be called and this function will add an entry with parent == 65535.
And the second problem is that when a vif is removed, no cleanup is made in cache entry. Hence, we can have a cache entry which points to an invalid vif (dev is set ot NULL).

Ok, I'll take another look at your patch, thanks for explaining.

David Miller

maintainer Applied

2010-03-27 15:33 UTC #4 lore raw

In reply to Nicolas Dichtel

From: Nicolas Dichtel <[email protected]>
Date: Fri, 26 Mar 2010 18:19:34 +0100

Quoted 7 lines

When a multicast packet arrived in ip6_mr_input(), if there is no cache ip6mr_cache_unresolved() will be called and this function will add an entry with parent == 65535.
And the second problem is that when a vif is removed, no cleanup is made in cache entry. Hence, we can have a cache entry which points to an invalid vif (dev is set ot NULL).

I've applied your fix, thanks Nicolas.