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

[Andrew Jones]

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 at redhat.com>
> Signed-off-by: Thomas Huth <thuth at redhat.com>
> ---
>  clients/net-snk/app/netlib/ipv6.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)

Reviewed-by: Andrew Jones <drjones at 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
>