[ovs-dev,v2] netdev: check for NULL fields in netdev_get_addrs

When the interfaces list is retrieved through getiffaddrs(), there
might be elements with iface_name set to NULL.

This patch checks ifa_name to be not NULL before comparing it to the
actual device name in the loop that calculates how many interfaces
exist with that same name.

Also, this patch checks that ifa_netmask is not NULL for coherence
with the existing code so that it doesn't allocate more memory
than needed if this field is NULL.

Note, that these checks are already being done later in the function
so it should be done in both places.

Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
---
I've been debugging a coredump produced by a segmentation fault of
ovs-vswitchd. It seems to be caused by a NULL pointer passed to
strncmp by netdev_get_addrs() function:

#0  0x00007fd840e2d34c in ?? () from /lib64/libc.so.6
#1  0x00007fd842ae63b6 in netdev_get_addrs (dev=0x7fd844e1e750 "vlan121",
paddr=paddr@entry=0x7ffe833244a0, pmask=pmask@entry=0x7ffe83324498,
n_in=n_in@entry=0x7ffe83324494)
    at lib/netdev.c:1890
#2  0x00007fd842b70365 in netdev_linux_get_addr_list
(netdev_=0x7fd844e8ec40, addr=0x7ffe833244a0, mask=0x7ffe83324498,
n_cnt=0x7ffe83324494) at lib/netdev-linux.c:2517
#3  0x00007fd842ae576f in netdev_get_addr_list (netdev=<optimized out>,
addr=addr@entry=0x7ffe833244a0, mask=mask@entry=0x7ffe83324498,
n_addr=n_addr@entry=0x7ffe83324494)
    at lib/netdev.c:1133
#4  0x00007fd842b30191 in get_src_addr (ip6_dst=ip6_dst@entry=0x7ffe8332522c,
output_bridge=output_bridge@entry=0x7ffe8332524c "vlan121", psrc=psrc@entry
=0x7fd844e6f0a0)
    at lib/ovs-router.c:146
#5  0x00007fd842b30655 in ovs_router_insert__ (priority=<optimized out>,
ip6_dst=ip6_dst@entry=0x7ffe8332522c, plen=<optimized out>,
    output_bridge=output_bridge@entry=0x7ffe8332524c "vlan121",
gw=gw@entry=0x7ffe8332523c)
at lib/ovs-router.c:200
#6  0x00007fd842b30e37 in ovs_router_insert
(ip_dst=ip_dst@entry=0x7ffe8332522c,
plen=<optimized out>, output_bridge=output_bridge@entry=0x7ffe8332524c
"vlan121",
    gw=gw@entry=0x7ffe8332523c) at lib/ovs-router.c:228
#7  0x00007fd842b79d24 in route_table_handle_msg (change=0x7ffe83325220) at
lib/route-table.c:295
#8  route_table_reset () at lib/route-table.c:174
#9  0x00007fd842b79ef5 in route_table_run () at lib/route-table.c:127
#10 0x00007fd842ae3701 in netdev_vport_run (netdev_class=<optimized out>)
at lib/netdev-vport.c:319
#11 0x00007fd842ae438e in netdev_run () at lib/netdev.c:163
#12 0x00007fd8428f329c in main (argc=10, argv=0x7ffe833265a8) at
vswitchd/ovs-vswitchd.c:114

In frame 1 we can confirm this:

(gdb) p *ifa
$94 = {ifa_next = 0x7fd8451c2a78, ifa_name = 0x0, ifa_flags = 0, ifa_addr =
0x7fd8451c29f8, ifa_netmask = 0x7fd8451c2a1c, ifa_ifu = {ifu_broadaddr =
0x0, ifu_dstaddr = 0x0}, ifa_data = 0x0}

(gdb) list
1885            if (ifa->ifa_addr != NULL) {
1886                int family;
1887
1888                family = ifa->ifa_addr->sa_family;
1889                if (family == AF_INET || family == AF_INET6) {
1890                    if (!strncmp(ifa->ifa_name, dev, IFNAMSIZ)) {
1891                        cnt++;
1892                    }
1893                }
1894            }


Later in the code, we're checking for ifa_name [0] not NULL so it
makes sense to check it as well where this patch does it.

Also, as it's not expected to get an unnamed interface, it
may happen and also iproute2 checks this condition when retrieving
the interfaces list via netlink [1].

[0] https://github.com/openvswitch/ovs/blob/master/lib/netdev.c#L1970
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/shemminger/iproute2.git/tree/ip/ipaddress.c#n664

 lib/netdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

             family = ifa->ifa_addr->sa_family;

--
1.8.3.1

Daniel Alvarez Sanchez <dalvarez@redhat.com> writes:

> When the interfaces list is retrieved through getiffaddrs(), there
> might be elements with iface_name set to NULL.
>
> This patch checks ifa_name to be not NULL before comparing it to the
> actual device name in the loop that calculates how many interfaces
> exist with that same name.
>
> Also, this patch checks that ifa_netmask is not NULL for coherence
> with the existing code so that it doesn't allocate more memory
> than needed if this field is NULL.
>
> Note, that these checks are already being done later in the function
> so it should be done in both places.
>
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

On 07/18/2017 05:25 PM, Daniel Alvarez Sanchez wrote:
> When the interfaces list is retrieved through getiffaddrs(), there
> might be elements with iface_name set to NULL.
> 
> This patch checks ifa_name to be not NULL before comparing it to the
> actual device name in the loop that calculates how many interfaces
> exist with that same name.
> 
> Also, this patch checks that ifa_netmask is not NULL for coherence
> with the existing code so that it doesn't allocate more memory
> than needed if this field is NULL.
> 
> Note, that these checks are already being done later in the function
> so it should be done in both places.
> 
> Signed-off-by: Daniel Alvarez <dalvarez@redhat.com>
> ---

Acked-by: Timothy Redaelli <tredaelli@redhat.com>