Message ID | 1500650904-2751-1-git-send-email-dalvarez@redhat.com |
---|---|
State | Accepted |
Headers | show |
> From: "Daniel Alvarez" <dalvarez@redhat.com> > To: dev@openvswitch.org > Sent: Friday, 21 July, 2017 11:28:24 AM > Subject: [ovs-dev] [PATCH v3] 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> > --- > v2 -> v3: fix email formatting since v2 wasn't correctly picked by > patchwork. > > 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(-) > > diff --git a/lib/netdev.c b/lib/netdev.c > index 0d5fad5..eed4d09 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 && ifa->ifa_netmask) { > int family; > > family = ifa->ifa_addr->sa_family; > -- > 1.8.3.1 > Makes sense, LGTM. Acked-by: Lance Richardson <lrichard@redhat.com>
On Fri, Jul 21, 2017 at 03:28:24PM +0000, Daniel Alvarez 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> > --- > v2 -> v3: fix email formatting since v2 wasn't correctly picked by > patchwork. Thanks. I applied this to master, branch-2.8, branch-2.7, and branch-2.6.
diff --git a/lib/netdev.c b/lib/netdev.c index 0d5fad5..eed4d09 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 && ifa->ifa_netmask) { int family; family = ifa->ifa_addr->sa_family;
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> --- v2 -> v3: fix email formatting since v2 wasn't correctly picked by patchwork. 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(-)