[SLOF] [PATCH 1/3] ipv6: Do not use unitialized MAC address array

[Alexey Kardashevskiy]

On 04/07/2016 08:09 PM, Thomas Huth wrote:
> The code in send_ipv6() currently basically looks like this:
>
> 	uint8_t *mac_addr, mac[6];
> 	mac_addr = mac;
> 	...
> 	n = find_neighbor (&ip_dst);
> 	if (ip6_is_multicast (&ip_dst)) {
> 		mac_addr = ip6_to_multicast_mac (&ip_dst, mac);
> 	}
> 	else {
> 		// Check if the MAC address is already cached
> 		if (n) {
> 			if (memcmp(n->mac, null_mac, ETH_ALEN) != 0)
> 				memcpy (mac_addr, &(n->mac), ETH_ALEN);
> 			/* XXX */
> 		}
> 		...
> 	}
> 	...
> 	fill_ethhdr (n->eth_frame, htons(ETHERTYPE_IPv6), get_mac_address(),
> 		     mac_addr);
>
> That means mac_addr initially points to the uninitialized mac[6]
> array on the stack. Now if there was already an entry in the neighbor
> cache, but the MAC address has not been determined yet, that
> uninitialized array could be used as final MAC address for fill_ethhdr()
> (since there is no "else" path at the spot marked with XXX above),
> resulting in random data in the MAC address field of the ethernet packet.
>
> Let's fix this issue by letting mac_addr point to the null_mac by
> default instead, so that it never points to invalid data. Also
> rename mac[6] to mc_mac[6] to make it clear that this array is
> only used for storing the multicast mac address.
>
> Signed-off-by: Thomas Huth <thuth at redhat.com>
> ---
>   clients/net-snk/app/netlib/ipv6.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/clients/net-snk/app/netlib/ipv6.c b/clients/net-snk/app/netlib/ipv6.c
> index 348e79d..5a307ca 100644
> --- a/clients/net-snk/app/netlib/ipv6.c
> +++ b/clients/net-snk/app/netlib/ipv6.c
> @@ -491,9 +491,9 @@ int send_ipv6(int fd, void* buffer, int len)
>   	struct udphdr *udph;
>   	struct icmp6hdr *icmp6h;
>   	ip6_addr_t ip_dst;
> -	uint8_t *mac_addr, mac[6];
> +	uint8_t *mac_addr, mc_mac[6];
>
> -	mac_addr = mac;
> +	mac_addr = null_mac;
>
>   	ip6h    = (struct ip6hdr *) buffer;
>   	udph   = (struct udphdr *)   ((uint8_t *) ip6h + sizeof (struct ip6hdr));
> @@ -526,13 +526,13 @@ int send_ipv6(int fd, void* buffer, int len)
>
>   	// If address is a multicast address, create a proper mac address
>   	if (ip6_is_multicast (&ip_dst)) {
> -		mac_addr = ip6_to_multicast_mac (&ip_dst, mac);
> +		mac_addr = ip6_to_multicast_mac (&ip_dst, mc_mac);
>   	}
>   	else {
>   		// Check if the MAC address is already cached
>   		if (n) {
>   			if (memcmp(n->mac, null_mac, ETH_ALEN) != 0)
> -				memcpy (mac_addr, &(n->mac), ETH_ALEN); /* found it */
> +				mac_addr = n->mac;		/* found it */


btw there is another thing - sometime the check if mac is NULL is done by 
"if (mac_addr == null_mac)", sometime by memcmp, can we unify this?


>   		} else if (!is_ip6addr_in_my_net(&ip_dst)) {
>   			struct router *gw;
>   			gw = ipv6_get_default_router(&ip6h->src);
>





-- 
Alexey