| Message ID | 20230202003042.1345433-1-hzhou@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev] northd.c: Validate port type to avoid unexpected behavior. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On 2/2/23 01:30, Han Zhou wrote: > In ovn_igmp_group_get_ports(), it accesses a union member that should > exist only if the port is a LSP: port->peer->od->mcast_info.rtr.relay. > But in theory it is possible that the "port" is in fact a LRP, because > it is a result of ovn_port_find() with the port name coming from a SB DB > entry. So it is better to validate that first. > > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- Good catch, thanks for the fix! Acked-by: Dumitru Ceara <dceara@redhat.com> Do you think it's worth backporting this? Regards, Dumitru
On Thu, Feb 2, 2023 at 11:51 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 2/2/23 01:30, Han Zhou wrote: > > In ovn_igmp_group_get_ports(), it accesses a union member that should > > exist only if the port is a LSP: port->peer->od->mcast_info.rtr.relay. > > But in theory it is possible that the "port" is in fact a LRP, because > > it is a result of ovn_port_find() with the port name coming from a SB DB > > entry. So it is better to validate that first. > > > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > Good catch, thanks for the fix! > > Acked-by: Dumitru Ceara <dceara@redhat.com> > > Do you think it's worth backporting this? > Thanks Dumitru! The problem is not easy to be triggered, but the change is small and it doesn't harm to backport. So, I applied to main and backported down to 22.03. Han > Regards, > Dumitru >
On 2/3/23 01:43, Han Zhou wrote: > On Thu, Feb 2, 2023 at 11:51 AM Dumitru Ceara <dceara@redhat.com> wrote: >> >> On 2/2/23 01:30, Han Zhou wrote: >>> In ovn_igmp_group_get_ports(), it accesses a union member that should >>> exist only if the port is a LSP: port->peer->od->mcast_info.rtr.relay. >>> But in theory it is possible that the "port" is in fact a LRP, because >>> it is a result of ovn_port_find() with the port name coming from a SB DB >>> entry. So it is better to validate that first. >>> >>> Signed-off-by: Han Zhou <hzhou@ovn.org> >>> --- >> >> Good catch, thanks for the fix! >> >> Acked-by: Dumitru Ceara <dceara@redhat.com> >> >> Do you think it's worth backporting this? >> > Thanks Dumitru! > The problem is not easy to be triggered, but the change is small and it > doesn't harm to backport. So, I applied to main and backported down to > 22.03. > Cool, thanks!
diff --git a/northd/northd.c b/northd/northd.c index 0944a7b5673f..2d82eccfeff8 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -4871,7 +4871,7 @@ ovn_igmp_group_get_ports(const struct sbrec_igmp_group *sb_igmp_group, struct ovn_port *port = ovn_port_find(ovn_ports, sb_igmp_group->ports[i]->logical_port); - if (!port) { + if (!port || !port->nbsp) { continue; }
In ovn_igmp_group_get_ports(), it accesses a union member that should exist only if the port is a LSP: port->peer->od->mcast_info.rtr.relay. But in theory it is possible that the "port" is in fact a LRP, because it is a result of ovn_port_find() with the port name coming from a SB DB entry. So it is better to validate that first. Signed-off-by: Han Zhou <hzhou@ovn.org> --- northd/northd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)