Skip to content

Commit

Permalink
pinctrl: Directly retrieve desired port_binding MAC.
Browse files Browse the repository at this point in the history
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>
Acked-by: Han Zhou <hzhou@ovn.org>
  • Loading branch information
putnopvut committed Jan 11, 2024
1 parent 9a0f307 commit 50f2262
Showing 1 changed file with 18 additions and 4 deletions.
22 changes: 18 additions & 4 deletions controller/pinctrl.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 50f2262

Please sign in to comment.