On Wed, Jan 10, 2024 at 11:26 AM Mark Michelson <[email protected]> wrote: > > A static analyzer determined that if pb->n_mac was 0, then the c_addrs > lport_addresses struct would never be initialized. We would then use > and attempt to free uninitialized memory. > > In reality, pb->n_mac should always be 1. This is because the port binding is a > representation of a northbound logical router port. Logical router ports do > not contain an array of MACs like the southbound port bindings do. Instead, > they have a single MAC string that is always guaranteed to be non-NULL. This > string is copied into the port binding's MAC array. Therefore, a southbound > port binding that comes from a logical router port will always have n_mac set > to 1. > > How do we know this is a logical router port? The ports iterated over in this > function must have IPv6 prefix delegation configured on them. Only northbound > logical router ports have this option available. > > To silence the static analyzer, this change directly retrieves pb->mac[0] > instead of iterating over the pb->mac array. As a safeguard, we ensure > that the port binding has only one MAC before attempting to access it. > This is based on the off chance that something other than northd has > inserted the port binding into the southbound database. > > Reported-at: https://issues.redhat.com/browse/FDP-224 > > Signed-off-by: Mark Michelson <[email protected]> > --- > controller/pinctrl.c | 22 ++++++++++++++++++---- > 1 file changed, 18 insertions(+), 4 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 12055a675..49a56cf81 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1286,11 +1286,25 @@ fill_ipv6_prefix_state(struct ovsdb_idl_txn *ovnsb_idl_txn, > continue; > } > > + /* To reach this point, the port binding must be a logical router > + * port. LRPs are configured with a single MAC that is always non-NULL. > + * Therefore, as long as we are working with a port_binding that was > + * inserted into the southbound database by northd, we can always > + * safely extract pb->mac[0] since it will be non-NULL. > + * > + * However, if a port_binding was inserted by someone else, then we > + * need to double-check our assumption first. > + */ > + if (pb->n_mac != 1) { > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > + VLOG_ERR_RL(&rl, "Port binding "UUID_FMT" has %"PRIuSIZE" MACs " > + "instead of 1", UUID_ARGS(&pb->header_.uuid), > + pb->n_mac); > + continue; > + } > struct lport_addresses c_addrs; > - for (size_t j = 0; j < pb->n_mac; j++) { > - if (extract_lsp_addresses(pb->mac[j], &c_addrs)) { > - break; > - } > + if (!extract_lsp_addresses(pb->mac[0], &c_addrs)) { > + continue; > } > > pfd = shash_find_data(&ipv6_prefixd, pb->logical_port); > -- > 2.40.1 >
Thanks Mark. Acked-by: Han Zhou <[email protected]> > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
