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

[Alexey Kardashevskiy]

On 04/12/2016 05:05 PM, Thomas Huth wrote:
> 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.


I am just reading the resulting code and afaict you could return from 
send_ipv6() right where you assign null_mac to mac_addr; otherwise I get 
impression that n->mac can possibly point to null_mac or 
ip6_to_multicast_mac() can return null_mac (but neither can).


> 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.


Sure, memcmp() is the way to go.


-- 
Alexey