diff mbox

[1/5] ipv6: Fix possible NULL-pointer dereference in send_ipv6()

Message ID 1462218931-4573-2-git-send-email-thuth@redhat.com
State Accepted
Headers show

Commit Message

Thomas Huth May 2, 2016, 7:55 p.m. UTC
The "struct neighbor *n" pointer in send_ipv6() can be NULL, e.g.
when we're sending to multicast addresses or to a server that sits
behind a router (since we do not have an entry in the neighbor cache
in this case).
However, the final code in send_ipv6() is always using n->eth_frame
to assemble the ethernet packet, and thus silently writes the data
into the low memory (which happens unnoticed because SLOF does not
use the MMU for memory protection).
This issue is now fixed by using a separate buffer for assembling
those ethernet packets instead. The block for using the router's
MAC address is also moved out of the block that is supposed to handle
the unicast transfers, so that we do not accidentially end up in the
neighbour solicitation code here (which also relies on n != NULL).

Reported-by: Andrew Jones <drjones@redhat.com>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 clients/net-snk/app/netlib/ipv6.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

Comments

Andrew Jones May 3, 2016, 5:11 a.m. UTC | #1
On Mon, May 02, 2016 at 09:55:27PM +0200, Thomas Huth wrote:
> The "struct neighbor *n" pointer in send_ipv6() can be NULL, e.g.
> when we're sending to multicast addresses or to a server that sits
> behind a router (since we do not have an entry in the neighbor cache
> in this case).
> However, the final code in send_ipv6() is always using n->eth_frame
> to assemble the ethernet packet, and thus silently writes the data
> into the low memory (which happens unnoticed because SLOF does not
> use the MMU for memory protection).
> This issue is now fixed by using a separate buffer for assembling
> those ethernet packets instead. The block for using the router's
> MAC address is also moved out of the block that is supposed to handle
> the unicast transfers, so that we do not accidentially end up in the
> neighbour solicitation code here (which also relies on n != NULL).
> 
> Reported-by: Andrew Jones <drjones@redhat.com>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  clients/net-snk/app/netlib/ipv6.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c
> index dfa16b3..6c041d6 100644
> --- a/clients/net-snk/app/netlib/ipv6.c
> +++ b/clients/net-snk/app/netlib/ipv6.c
> @@ -488,12 +488,12 @@ static unsigned short ip6_checksum(struct ip6hdr *ip6h, unsigned short *packet,
>   */
>  int send_ipv6(int fd, void* buffer, int len)
>  {
> -	struct neighbor *n;
>  	struct ip6hdr *ip6h;
>  	struct udphdr *udph;
>  	struct icmp6hdr *icmp6h;
>  	ip6_addr_t ip_dst;
>  	uint8_t *mac_addr, mc_mac[6];
> +	static uint8_t ethframe[ETH_MTU_SIZE];
>  
>  	mac_addr = null_mac;
>  
> @@ -524,21 +524,20 @@ int send_ipv6(int fd, void* buffer, int len)
>  						 (unsigned short *) icmp6h,
>  						 ip6h->pl >> 1);
>  
> -	n = find_neighbor (&ip_dst);
> -
> -	// If address is a multicast address, create a proper mac address
>  	if (ip6_is_multicast (&ip_dst)) {
> +		/* If multicast, then create a proper multicast mac address */
>  		mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac);
> -	}
> -	else {
> -		// Check if the MAC address is already cached
> -		if (n) {
> +	} else if (!is_ip6addr_in_my_net(&ip_dst)) {
> +		/* If server is not in same subnet, user MAC of the router */
> +		struct router *gw;
> +		gw = ipv6_get_default_router(&ip6h->src);
> +		mac_addr = gw ? gw->mac : null_mac;
> +	} else {
> +		/* Normal unicast, so use neighbor cache to look up MAC */
> +		struct neighbor *n = find_neighbor (&ip_dst);
> +		if (n) {				/* Already cached ? */
>  			if (memcmp(n->mac, null_mac, ETH_ALEN) != 0)
>  				mac_addr = n->mac;		/* found it */
> -		} else if (!is_ip6addr_in_my_net(&ip_dst)) {
> -			struct router *gw;
> -			gw = ipv6_get_default_router(&ip6h->src);
> -			mac_addr = gw ? gw->mac : null_mac;
>  		} else {
>  			mac_addr = null_mac;
>  			n = malloc(sizeof(struct neighbor));
> @@ -575,10 +574,11 @@ int send_ipv6(int fd, void* buffer, int len)
>  	if (mac_addr == null_mac)
>  		return -1;
>  
> -	fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(),
> -		     mac_addr);
> -	memcpy (&(n->eth_frame[sizeof(struct ethhdr)]), buffer, len);
> -	return send_ether (fd, n->eth_frame, len + sizeof(struct ethhdr));
> +	fill_ethhdr(ethframe, htons(ETHERTYPE_IPv6), get_mac_address(),
> +	            mac_addr);
> +	memcpy(&ethframe[sizeof(struct ethhdr)], buffer, len);
> +
> +	return send_ether(fd, ethframe, len + sizeof(struct ethhdr));
>  }
>  
>  static int check_colons(const char *str)
> -- 
> 1.8.3.1
>
diff mbox

Patch

diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c
index dfa16b3..6c041d6 100644
--- a/clients/net-snk/app/netlib/ipv6.c
+++ b/clients/net-snk/app/netlib/ipv6.c
@@ -488,12 +488,12 @@  static unsigned short ip6_checksum(struct ip6hdr *ip6h, unsigned short *packet,
  */
 int send_ipv6(int fd, void* buffer, int len)
 {
-	struct neighbor *n;
 	struct ip6hdr *ip6h;
 	struct udphdr *udph;
 	struct icmp6hdr *icmp6h;
 	ip6_addr_t ip_dst;
 	uint8_t *mac_addr, mc_mac[6];
+	static uint8_t ethframe[ETH_MTU_SIZE];
 
 	mac_addr = null_mac;
 
@@ -524,21 +524,20 @@  int send_ipv6(int fd, void* buffer, int len)
 						 (unsigned short *) icmp6h,
 						 ip6h->pl >> 1);
 
-	n = find_neighbor (&ip_dst);
-
-	// If address is a multicast address, create a proper mac address
 	if (ip6_is_multicast (&ip_dst)) {
+		/* If multicast, then create a proper multicast mac address */
 		mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac);
-	}
-	else {
-		// Check if the MAC address is already cached
-		if (n) {
+	} else if (!is_ip6addr_in_my_net(&ip_dst)) {
+		/* If server is not in same subnet, user MAC of the router */
+		struct router *gw;
+		gw = ipv6_get_default_router(&ip6h->src);
+		mac_addr = gw ? gw->mac : null_mac;
+	} else {
+		/* Normal unicast, so use neighbor cache to look up MAC */
+		struct neighbor *n = find_neighbor (&ip_dst);
+		if (n) {				/* Already cached ? */
 			if (memcmp(n->mac, null_mac, ETH_ALEN) != 0)
 				mac_addr = n->mac;		/* found it */
-		} else if (!is_ip6addr_in_my_net(&ip_dst)) {
-			struct router *gw;
-			gw = ipv6_get_default_router(&ip6h->src);
-			mac_addr = gw ? gw->mac : null_mac;
 		} else {
 			mac_addr = null_mac;
 			n = malloc(sizeof(struct neighbor));
@@ -575,10 +574,11 @@  int send_ipv6(int fd, void* buffer, int len)
 	if (mac_addr == null_mac)
 		return -1;
 
-	fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(),
-		     mac_addr);
-	memcpy (&(n->eth_frame[sizeof(struct ethhdr)]), buffer, len);
-	return send_ether (fd, n->eth_frame, len + sizeof(struct ethhdr));
+	fill_ethhdr(ethframe, htons(ETHERTYPE_IPv6), get_mac_address(),
+	            mac_addr);
+	memcpy(&ethframe[sizeof(struct ethhdr)], buffer, len);
+
+	return send_ether(fd, ethframe, len + sizeof(struct ethhdr));
 }
 
 static int check_colons(const char *str)