| Message ID | 20240110192633.14469-1-mmichels@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] pinctrl: Directly retrieve desired port_binding MAC. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Wed, Jan 10, 2024 at 11:26 AM Mark Michelson <mmichels@redhat.com> 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 <mmichels@redhat.com> > --- > 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 <hzhou@ovn.org> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1/10/24 19:05, Han Zhou wrote: > > > On Wed, Jan 10, 2024 at 11:26 AM Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> 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 > <https://issues.redhat.com/browse/FDP-224> > > > > Signed-off-by: Mark Michelson <mmichels@redhat.com > <mailto:mmichels@redhat.com>> > > --- > > 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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> Thanks for the review, Han. I pushed this change to main and all branches back to 22.03. > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
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);
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 <mmichels@redhat.com> --- controller/pinctrl.c | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-)