diff mbox

[ovs-dev] netdev: check for iface_name not NULL in netdev_get_addrs

Message ID CAPXMuf_uAOq9pen4z9LzgmVBzWwYUR7ftNV+28jMc33K09MK7A@mail.gmail.com
State Accepted
Headers show

Commit Message

Daniel Alvarez Sanchez July 17, 2017, 12:57 p.m. UTC 
When the interfaces list is retrieved through getiffaddrs(), there
might be elements with iface_name set to NULL.
This patch checks iface_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.

Note, that this check is 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.

[0] https://github.com/openvswitch/ovs/blob/master/lib/netdev.c#L1970


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

             family = ifa->ifa_addr->sa_family;

--
1.8.3.1

Comments

Aaron Conole July 17, 2017, 7:07 p.m. UTC | #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 iface_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.
>
> Note, that this check is 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.
>
> [0] https://github.com/openvswitch/ovs/blob/master/lib/netdev.c#L1970
>
>
>  lib/netdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 0d5fad5..af8ceb7 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -1946,7 +1946,7 @@ netdev_get_addrs(const char dev[], struct in6_addr
> **paddr,
>      }
>
>      for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
> -        if (ifa->ifa_addr != NULL) {
> +        if (ifa->ifa_addr && ifa->ifa_name) {
>              int family;
>
>              family = ifa->ifa_addr->sa_family;
>
> --
> 1.8.3.1

Good catch.

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

A future cleanup might be to make the whole thing a single loop using
xrealloc to build the return array.  I think it would read a bit easier,
and get rid of these kinds of accidental redundant checks, but it is
outside the scope of this change.
Daniel Alvarez Sanchez July 18, 2017, 3:27 p.m. UTC | #2 
Thanks Aaron, I just sent a new version of it which also checks for
ifa_netmask
as the existing code does currently. It will save some memory in cases where
either name or netmask are NULL (which I guess it shouldn't happen but it
can
happen).

Thanks again,
Daniel

On Mon, Jul 17, 2017 at 9:07 PM, Aaron Conole <aconole@redhat.com> wrote:

> 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 iface_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.
> >
> > Note, that this check is 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.
> >
> > [0] https://github.com/openvswitch/ovs/blob/master/lib/netdev.c#L1970
> >
> >
> >  lib/netdev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/netdev.c b/lib/netdev.c
> > index 0d5fad5..af8ceb7 100644
> > --- a/lib/netdev.c
> > +++ b/lib/netdev.c
> > @@ -1946,7 +1946,7 @@ netdev_get_addrs(const char dev[], struct in6_addr
> > **paddr,
> >      }
> >
> >      for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
> > -        if (ifa->ifa_addr != NULL) {
> > +        if (ifa->ifa_addr && ifa->ifa_name) {
> >              int family;
> >
> >              family = ifa->ifa_addr->sa_family;
> >
> > --
> > 1.8.3.1
>
> Good catch.
>
> Acked-by: Aaron Conole <aconole@redhat.com>
>
> A future cleanup might be to make the whole thing a single loop using
> xrealloc to build the return array.  I think it would read a bit easier,
> and get rid of these kinds of accidental redundant checks, but it is
> outside the scope of this change.
>
diff mbox

Patch

diff --git a/lib/netdev.c b/lib/netdev.c
index 0d5fad5..af8ceb7 100644
--- a/lib/netdev.c
+++ b/lib/netdev.c
@@ -1946,7 +1946,7 @@  netdev_get_addrs(const char dev[], struct in6_addr
**paddr,
     }

     for (ifa = if_addr_list; ifa; ifa = ifa->ifa_next) {
-        if (ifa->ifa_addr != NULL) {
+        if (ifa->ifa_addr && ifa->ifa_name) {
             int family;