[ovs-dev,ovn] ovn-northd: Validate dnat_and_snat external_mac/logical_ip.
diff mbox series

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.
Related show

Commit Message

Dumitru Ceara Nov. 7, 2019, 12:19 p.m. UTC
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(-)

Comments

Numan Siddique Nov. 7, 2019, 2:44 p.m. UTC | #1
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
Dumitru Ceara Nov. 7, 2019, 3:43 p.m. UTC | #2
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

Patch
diff mbox series

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];