diff mbox series

[ovs-dev,v12,6/8] ovs-vsctl: Fix crash when routing is enabled

Message ID 20230613183443.31540-7-jamestiotio@gmail.com
State Accepted, archived
Commit b2d45921a674dd0227225525966bb04f5abb3e55
Headers show
Series treewide: Fix multiple Coverity defects | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/intel-ovs-compilation success test: success

Commit Message

James Raphael Tiovalen June 13, 2023, 6:34 p.m. UTC 
In the case where routing is enabled, the bridge member of the
`vsctl_port` structs is not populated. This can cause a crash if we
attempt to access it. This patch fixes the crash by checking if the
bridge member is valid before attempting to access it. In the
`check_conflicts` function, we print both the port name and the bridge
name if routing is disabled and we only print the port name if routing
is enabled.

Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
Reviewed-by: Simon Horman <simon.horman@corigine.com>
---
 utilities/ovs-vsctl.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

Comments

Eelco Chaudron July 11, 2023, 10:22 a.m. UTC | #1 
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:

> In the case where routing is enabled, the bridge member of the
> `vsctl_port` structs is not populated. This can cause a crash if we
> attempt to access it. This patch fixes the crash by checking if the
> bridge member is valid before attempting to access it. In the
> `check_conflicts` function, we print both the port name and the bridge
> name if routing is disabled and we only print the port name if routing
> is enabled.
>
> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
> Reviewed-by: Simon Horman <simon.horman@corigine.com>

Thank for fixing this James, this patch looks good to me.

Acked-by: Eelco Chaudron <echaudro@redhat.com>
Ilya Maximets July 12, 2023, 12:10 a.m. UTC | #2 
On 7/11/23 12:22, Eelco Chaudron wrote:
> 
> 
> On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> 
>> In the case where routing is enabled, the bridge member of the
>> `vsctl_port` structs is not populated. This can cause a crash if we
>> attempt to access it. This patch fixes the crash by checking if the
>> bridge member is valid before attempting to access it. In the
>> `check_conflicts` function, we print both the port name and the bridge
>> name if routing is disabled and we only print the port name if routing
>> is enabled.
>>
>> Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com>
>> Reviewed-by: Simon Horman <simon.horman@corigine.com>
> 
> Thank for fixing this James, this patch looks good to me.
> 
> Acked-by: Eelco Chaudron <echaudro@redhat.com>

Thanks, James, Simon and Eelco!

Applied this one.

Best regards, Ilya Maximets.
diff mbox series

Patch

diff --git a/utilities/ovs-vsctl.c b/utilities/ovs-vsctl.c
index b3c75f8ba..f55c2965a 100644
--- a/utilities/ovs-vsctl.c
+++ b/utilities/ovs-vsctl.c
@@ -888,14 +888,23 @@  check_conflicts(struct vsctl_context *vsctl_ctx, const char *name,
 
     port = shash_find_data(&vsctl_ctx->ports, name);
     if (port) {
-        ctl_fatal("%s because a port named %s already exists on "
-                    "bridge %s", msg, name, port->bridge->name);
+        if (port->bridge) {
+            ctl_fatal("%s because a port named %s already exists on "
+                      "bridge %s", msg, name, port->bridge->name);
+        } else {
+            ctl_fatal("%s because a port named %s already exists", msg, name);
+        }
     }
 
     iface = shash_find_data(&vsctl_ctx->ifaces, name);
     if (iface) {
-        ctl_fatal("%s because an interface named %s already exists "
-                    "on bridge %s", msg, name, iface->port->bridge->name);
+        if (iface->port->bridge) {
+            ctl_fatal("%s because an interface named %s already exists "
+                      "on bridge %s", msg, name, iface->port->bridge->name);
+        } else {
+            ctl_fatal("%s because an interface named %s already exists", msg,
+                      name);
+        }
     }
 
     free(msg);
@@ -935,7 +944,7 @@  find_port(struct vsctl_context *vsctl_ctx, const char *name, bool must_exist)
     ovs_assert(vsctl_ctx->cache_valid);
 
     port = shash_find_data(&vsctl_ctx->ports, name);
-    if (port && !strcmp(name, port->bridge->name)) {
+    if (port && port->bridge && !strcmp(name, port->bridge->name)) {
         port = NULL;
     }
     if (must_exist && !port) {
@@ -953,7 +962,8 @@  find_iface(struct vsctl_context *vsctl_ctx, const char *name, bool must_exist)
     ovs_assert(vsctl_ctx->cache_valid);
 
     iface = shash_find_data(&vsctl_ctx->ifaces, name);
-    if (iface && !strcmp(name, iface->port->bridge->name)) {
+    if (iface && iface->port->bridge &&
+        !strcmp(name, iface->port->bridge->name)) {
         iface = NULL;
     }
     if (must_exist && !iface) {