On 1/10/24 19:05, Han Zhou wrote:
On Wed, Jan 10, 2024 at 11:26 AM Mark Michelson <mmich...@redhat.com
<mailto:mmich...@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 <mmich...@redhat.com
<mailto:mmich...@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 <hz...@ovn.org <mailto:hz...@ovn.org>>
Thanks for the review, Han. I pushed this change to main and all
branches back to 22.03.
> _______________________________________________
> dev mailing list
> d...@openvswitch.org <mailto:d...@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
<https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
