| Message ID | 20201014170821.227967-1-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] ovn-ic: Fix route hash. | expand |
On Wed, Oct 14, 2020 at 10:38 PM Han Zhou <hzhou@ovn.org> wrote: > > The 'nexthop' that passed to ic_route_hash() is not fully initialized in > get_nexthop_from_lport_addresses(). 'nexthop' has type of 'struct v46_ip' which > contains a union to share space for ipv4 and ipv6 address. If only ipv4 > initialized where is a plenty of uninitialized space that goes to > hash_bytes(nexthop, sizeof *nexthop, basis). > > Impact: there are two places where this function is called. > > 1. In add_to_routes_ad(), the nexthop is initialized in parse_route() before > calling get_nexthop_from_lport_addresses(), luckily. > > 2. In add_network_to_routes_ad(), we are unlucky. When a directly connected > network of a router is found to be advertised, if the route already existed in > the global IC-SB, it may not be found due to the hash difference, and results > in the existing route being deleted and the same one recreated, unnecessarily. > > This patch fixes the problem by initializing the struct to zero before setting > the fields. > > From Ilya's report: > > Report from MemorySanitizer: > > > > ==3074629==WARNING: MemorySanitizer: use-of-uninitialized-value > > #0 0x67177e in mhash_add__ ovs/./lib/hash.h:66:9 > > #1 0x671668 in mhash_add ovs/./lib/hash.h:78:12 > > #2 0x6701e9 in hash_bytes ovs/lib/hash.c:38:16 > > #3 0x524b4a in add_network_to_routes_ad ic/ovn-ic.c:1095:5 > > #4 0x51eea3 in route_run ic/ovn-ic.c:1424:21 > > #5 0x51887b in main ic/ovn-ic.c:1674:17 > > #6 0x7fd4ce7871a2 in __libc_start_main > > #7 0x49c90d in _start (ic/ovn-ic+0x49c90d) > > > > Uninitialized value was created by an allocation of 'nexthop' in the > > stack frame of function 'add_network_to_routes_ad' > > #0 0x5245f0 in add_network_to_routes_ad ic/ovn-ic.c:1069 > > Reported-by: Ilya Maximets <i.maximets@ovn.org> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/376160.html > Fixes: 57b347c55 ("ovn-ic: Route advertisement.") > Signed-off-by: Han Zhou <hzhou@ovn.org> Acked-by: Numan Siddique <numans@ovn.org> Thanks Numan > --- > ic/ovn-ic.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > index dc9bcc64e..923969fff 100644 > --- a/ic/ovn-ic.c > +++ b/ic/ovn-ic.c > @@ -907,6 +907,7 @@ get_nexthop_from_lport_addresses(int family, > const struct lport_addresses *laddr, > struct v46_ip *nexthop) > { > + memset(nexthop, 0, sizeof *nexthop); > nexthop->family = family; > if (family == AF_INET) { > if (!laddr->n_ipv4_addrs) { > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Wed, Oct 21, 2020 at 6:38 AM Numan Siddique <numans@ovn.org> wrote: > > On Wed, Oct 14, 2020 at 10:38 PM Han Zhou <hzhou@ovn.org> wrote: > > > > The 'nexthop' that passed to ic_route_hash() is not fully initialized in > > get_nexthop_from_lport_addresses(). 'nexthop' has type of 'struct v46_ip' which > > contains a union to share space for ipv4 and ipv6 address. If only ipv4 > > initialized where is a plenty of uninitialized space that goes to > > hash_bytes(nexthop, sizeof *nexthop, basis). > > > > Impact: there are two places where this function is called. > > > > 1. In add_to_routes_ad(), the nexthop is initialized in parse_route() before > > calling get_nexthop_from_lport_addresses(), luckily. > > > > 2. In add_network_to_routes_ad(), we are unlucky. When a directly connected > > network of a router is found to be advertised, if the route already existed in > > the global IC-SB, it may not be found due to the hash difference, and results > > in the existing route being deleted and the same one recreated, unnecessarily. > > > > This patch fixes the problem by initializing the struct to zero before setting > > the fields. > > > > From Ilya's report: > > > Report from MemorySanitizer: > > > > > > ==3074629==WARNING: MemorySanitizer: use-of-uninitialized-value > > > #0 0x67177e in mhash_add__ ovs/./lib/hash.h:66:9 > > > #1 0x671668 in mhash_add ovs/./lib/hash.h:78:12 > > > #2 0x6701e9 in hash_bytes ovs/lib/hash.c:38:16 > > > #3 0x524b4a in add_network_to_routes_ad ic/ovn-ic.c:1095:5 > > > #4 0x51eea3 in route_run ic/ovn-ic.c:1424:21 > > > #5 0x51887b in main ic/ovn-ic.c:1674:17 > > > #6 0x7fd4ce7871a2 in __libc_start_main > > > #7 0x49c90d in _start (ic/ovn-ic+0x49c90d) > > > > > > Uninitialized value was created by an allocation of 'nexthop' in the > > > stack frame of function 'add_network_to_routes_ad' > > > #0 0x5245f0 in add_network_to_routes_ad ic/ovn-ic.c:1069 > > > > Reported-by: Ilya Maximets <i.maximets@ovn.org> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/376160.html > > Fixes: 57b347c55 ("ovn-ic: Route advertisement.") > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > Acked-by: Numan Siddique <numans@ovn.org> > > Thanks > Numan > > > --- > > ic/ovn-ic.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c > > index dc9bcc64e..923969fff 100644 > > --- a/ic/ovn-ic.c > > +++ b/ic/ovn-ic.c > > @@ -907,6 +907,7 @@ get_nexthop_from_lport_addresses(int family, > > const struct lport_addresses *laddr, > > struct v46_ip *nexthop) > > { > > + memset(nexthop, 0, sizeof *nexthop); > > nexthop->family = family; > > if (family == AF_INET) { > > if (!laddr->n_ipv4_addrs) { > > -- > > 2.26.2 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks Numan. I applied this to master and backported up to branch 20.03. Han
diff --git a/ic/ovn-ic.c b/ic/ovn-ic.c index dc9bcc64e..923969fff 100644 --- a/ic/ovn-ic.c +++ b/ic/ovn-ic.c @@ -907,6 +907,7 @@ get_nexthop_from_lport_addresses(int family, const struct lport_addresses *laddr, struct v46_ip *nexthop) { + memset(nexthop, 0, sizeof *nexthop); nexthop->family = family; if (family == AF_INET) { if (!laddr->n_ipv4_addrs) {