| Message ID | 1573129159-14644-1-git-send-email-dceara@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,ovn] ovn-northd: Validate dnat_and_snat external_mac/logical_ip. | expand |
On Thu, Nov 7, 2019 at 5:52 PM Dumitru Ceara <dceara@redhat.com> wrote: > > When dnat_and_snat NAT rules are configured, if the user tries to set > external_mac in the NAT rule record without setting logical_ip > ovn-northd crashes as there's no validation in place. > > Add checks for valid ethernet address in NAT.external_mac and for > non-null NAT.logical_ip where applicable. > > Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1769709 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks Dumitru. The fix looks good to me. I applied this to master. Thanks Numan > --- > northd/ovn-northd.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index c23c270..2f0f501 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -6032,9 +6032,12 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) > for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > const struct nbrec_nat *nat = op->od->nbr->nat[i]; > bool found = false; > + struct eth_addr mac; > > if (strcmp(nat->type, "dnat_and_snat") || > - !nat->external_mac || !nat->external_ip) { > + !nat->external_mac || > + !eth_addr_from_string(nat->external_mac, &mac) || > + !nat->external_ip || !nat->logical_port) { > continue; > } > > @@ -6083,10 +6086,14 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) > > for (size_t j = 0; j < op->od->nbr->n_nat; j++) { > const struct nbrec_nat *nat2 = op->od->nbr->nat[j]; > + struct eth_addr mac2; > > if (nat == nat2 || strcmp(nat2->type, "dnat_and_snat") || > - !nat2->external_mac || !nat2->external_ip) > + !nat2->external_mac || > + !eth_addr_from_string(nat2->external_mac, &mac2) || > + !nat2->external_ip) { > continue; > + } > > family = AF_INET; > if (!ip_parse(nat2->external_ip, &ip) || !ip) { > @@ -7785,7 +7792,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > if (od->l3dgw_port) { > /* Distributed router. */ > if (!strcmp(nat->type, "dnat_and_snat") && > - nat->external_mac && nat->external_ip) { > + nat->external_mac && nat->external_ip && > + eth_addr_from_string(nat->external_mac, &mac)) { > for (int j = 0; j < od->nbr->n_nat; j++) { > const struct nbrec_nat *nat2 = od->nbr->nat[j]; > > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Thu, Nov 7, 2019 at 3:44 PM Numan Siddique <numans@ovn.org> wrote: > > On Thu, Nov 7, 2019 at 5:52 PM Dumitru Ceara <dceara@redhat.com> wrote: > > > > When dnat_and_snat NAT rules are configured, if the user tries to set > > external_mac in the NAT rule record without setting logical_ip > > ovn-northd crashes as there's no validation in place. > > > > Add checks for valid ethernet address in NAT.external_mac and for > > non-null NAT.logical_ip where applicable. > > > > Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1769709 > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > > Thanks Dumitru. > The fix looks good to me. I applied this to master. > > Thanks > Numan Thanks Numan! > > > --- > > northd/ovn-northd.c | 14 +++++++++++--- > > 1 file changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > > index c23c270..2f0f501 100644 > > --- a/northd/ovn-northd.c > > +++ b/northd/ovn-northd.c > > @@ -6032,9 +6032,12 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) > > for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > > const struct nbrec_nat *nat = op->od->nbr->nat[i]; > > bool found = false; > > + struct eth_addr mac; > > > > if (strcmp(nat->type, "dnat_and_snat") || > > - !nat->external_mac || !nat->external_ip) { > > + !nat->external_mac || > > + !eth_addr_from_string(nat->external_mac, &mac) || > > + !nat->external_ip || !nat->logical_port) { > > continue; > > } > > > > @@ -6083,10 +6086,14 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) > > > > for (size_t j = 0; j < op->od->nbr->n_nat; j++) { > > const struct nbrec_nat *nat2 = op->od->nbr->nat[j]; > > + struct eth_addr mac2; > > > > if (nat == nat2 || strcmp(nat2->type, "dnat_and_snat") || > > - !nat2->external_mac || !nat2->external_ip) > > + !nat2->external_mac || > > + !eth_addr_from_string(nat2->external_mac, &mac2) || > > + !nat2->external_ip) { > > continue; > > + } > > > > family = AF_INET; > > if (!ip_parse(nat2->external_ip, &ip) || !ip) { > > @@ -7785,7 +7792,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, > > if (od->l3dgw_port) { > > /* Distributed router. */ > > if (!strcmp(nat->type, "dnat_and_snat") && > > - nat->external_mac && nat->external_ip) { > > + nat->external_mac && nat->external_ip && > > + eth_addr_from_string(nat->external_mac, &mac)) { > > for (int j = 0; j < od->nbr->n_nat; j++) { > > const struct nbrec_nat *nat2 = od->nbr->nat[j]; > > > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index c23c270..2f0f501 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -6032,9 +6032,12 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) for (size_t i = 0; i < op->od->nbr->n_nat; i++) { const struct nbrec_nat *nat = op->od->nbr->nat[i]; bool found = false; + struct eth_addr mac; if (strcmp(nat->type, "dnat_and_snat") || - !nat->external_mac || !nat->external_ip) { + !nat->external_mac || + !eth_addr_from_string(nat->external_mac, &mac) || + !nat->external_ip || !nat->logical_port) { continue; } @@ -6083,10 +6086,14 @@ add_distributed_nat_routes(struct hmap *lflows, const struct ovn_port *op) for (size_t j = 0; j < op->od->nbr->n_nat; j++) { const struct nbrec_nat *nat2 = op->od->nbr->nat[j]; + struct eth_addr mac2; if (nat == nat2 || strcmp(nat2->type, "dnat_and_snat") || - !nat2->external_mac || !nat2->external_ip) + !nat2->external_mac || + !eth_addr_from_string(nat2->external_mac, &mac2) || + !nat2->external_ip) { continue; + } family = AF_INET; if (!ip_parse(nat2->external_ip, &ip) || !ip) { @@ -7785,7 +7792,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, if (od->l3dgw_port) { /* Distributed router. */ if (!strcmp(nat->type, "dnat_and_snat") && - nat->external_mac && nat->external_ip) { + nat->external_mac && nat->external_ip && + eth_addr_from_string(nat->external_mac, &mac)) { for (int j = 0; j < od->nbr->n_nat; j++) { const struct nbrec_nat *nat2 = od->nbr->nat[j];
When dnat_and_snat NAT rules are configured, if the user tries to set external_mac in the NAT rule record without setting logical_ip ovn-northd crashes as there's no validation in place. Add checks for valid ethernet address in NAT.external_mac and for non-null NAT.logical_ip where applicable. Reported-by: Daniel Alvarez Sanchez <dalvarez@redhat.com> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1769709 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- northd/ovn-northd.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)