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

[Thomas Huth]

On 12.04.2016 05:43, Alexey Kardashevskiy wrote:
> 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?

The mac_addr == null_mac checks are of course only useful for the case
where mac_addr has been set to null_mac in this function before. The
memcmp is needed in the above chunk since it checks n->mac instead. So
if you want to unify all checks, I think you have to convert them to
memcmp(), but it won't work the other way round.

 Thomas