[ovs-dev] dpctl: Fix jump through wild pointer in "dpctl/help".
diff mbox

Message ID 1445117041-16814-1-git-send-email-blp@nicira.com
State Accepted
Headers show

Commit Message

Ben Pfaff Oct. 17, 2015, 9:24 p.m. UTC
dpctl_unixctl_handler() didn't fully initialize the dpctl_params structure
it passed to the handler, which meant that dpctl_help() could see a nonnull
(indeterminate) 'usage' pointer and jump through it, causes a crash.
This commit fixes the crash by fully initializing the structure.

The dpctl/help command wasn't going to do anything useful anyway, so this
commit also stops registering it.

Reported-by: Murali R <muralirdev@gmail.com>
Reported-at: http://openvswitch.org/pipermail/discuss/2015-October/019135.html
Signed-off-by: Ben Pfaff <blp@nicira.com>
---
 AUTHORS     |  1 +
 lib/dpctl.c | 27 ++++++++++++---------------
 2 files changed, 13 insertions(+), 15 deletions(-)

Comments

Daniele Di Proietto Oct. 20, 2015, 10:55 p.m. UTC | #1
On 17/10/2015 14:24, "Ben Pfaff" <blp@nicira.com> wrote:

>dpctl_unixctl_handler() didn't fully initialize the dpctl_params structure
>it passed to the handler, which meant that dpctl_help() could see a
>nonnull
>(indeterminate) 'usage' pointer and jump through it, causes a crash.
>This commit fixes the crash by fully initializing the structure.
>
>The dpctl/help command wasn't going to do anything useful anyway, so this
>commit also stops registering it.
>
>Reported-by: Murali R <muralirdev@gmail.com>
>Reported-at: 
>https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_piperm
>ail_discuss_2015-2DOctober_019135.html&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJA
>XVeAw-YihVMNtXt-uEs&r=SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=OrWGS3
>tQ3PzCvpaif3pjUIuOxAY8nQCTY_8Q_Qs_Wj8&s=TeEaw1_lnbTmgin2fd7Sjw0dEf__XAROJU
>z7KORhS1s&e= 
>Signed-off-by: Ben Pfaff <blp@nicira.com>

Acked-by: Daniele Di Proietto <diproiettod@vmware.com>
Ben Pfaff Nov. 4, 2015, 1:41 a.m. UTC | #2
On Tue, Oct 20, 2015 at 10:55:37PM +0000, Daniele Di Proietto wrote:
> 
> On 17/10/2015 14:24, "Ben Pfaff" <blp@nicira.com> wrote:
> 
> >dpctl_unixctl_handler() didn't fully initialize the dpctl_params structure
> >it passed to the handler, which meant that dpctl_help() could see a
> >nonnull
> >(indeterminate) 'usage' pointer and jump through it, causes a crash.
> >This commit fixes the crash by fully initializing the structure.
> >
> >The dpctl/help command wasn't going to do anything useful anyway, so this
> >commit also stops registering it.
> >
> >Reported-by: Murali R <muralirdev@gmail.com>
> >Reported-at: 
> >https://urldefense.proofpoint.com/v2/url?u=http-3A__openvswitch.org_piperm
> >ail_discuss_2015-2DOctober_019135.html&d=BQIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJA
> >XVeAw-YihVMNtXt-uEs&r=SmB5nZacmXNq0gKCC1s_Cw5yUNjxgD4v5kJqZ2uWLlE&m=OrWGS3
> >tQ3PzCvpaif3pjUIuOxAY8nQCTY_8Q_Qs_Wj8&s=TeEaw1_lnbTmgin2fd7Sjw0dEf__XAROJU
> >z7KORhS1s&e= 
> >Signed-off-by: Ben Pfaff <blp@nicira.com>
> 
> Acked-by: Daniele Di Proietto <diproiettod@vmware.com>

Thanks Daniele, I applied this to master and branch-2.4.

Patch
diff mbox

diff --git a/AUTHORS b/AUTHORS
index f4e1ca9..41264ec 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -328,6 +328,7 @@  Mike Kruze              mkruze@nicira.com
 Min Chen                ustcer.tonychan@gmail.com
 Mikael Doverhag         mdoverhag@nicira.com
 Mrinmoy Das             mrdas@ixiacom.com
+Murali R                muralirdev@gmail.com
 Nagi Reddy Jonnala      njonnala@Brocade.com
 Niels van Adrichem      N.L.M.vanAdrichem@tudelft.nl
 Niklas Andersson        nandersson@nicira.com
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 48bf6bc..438bfd3 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1583,15 +1583,13 @@  dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
                       void *aux)
 {
     struct ds ds = DS_EMPTY_INITIALIZER;
-    struct dpctl_params dpctl_p;
     bool error = false;
 
-    dpctl_command_handler *handler = (dpctl_command_handler *) aux;
-
-    dpctl_p.print_statistics = false;
-    dpctl_p.zero_statistics = false;
-    dpctl_p.may_create = false;
-    dpctl_p.verbosity = 0;
+    struct dpctl_params dpctl_p = {
+        .is_appctl = true,
+        .output = dpctl_unixctl_print,
+        .aux = &ds,
+    };
 
     /* Parse options (like getopt). Unfortunately it does
      * not seem a good idea to call getopt_long() here, since it uses global
@@ -1644,10 +1642,7 @@  dpctl_unixctl_handler(struct unixctl_conn *conn, int argc, const char *argv[],
     }
 
     if (!error) {
-        dpctl_p.is_appctl = true;
-        dpctl_p.output = dpctl_unixctl_print;
-        dpctl_p.aux = &ds;
-
+        dpctl_command_handler *handler = (dpctl_command_handler *) aux;
         error = handler(argc, argv, &dpctl_p) != 0;
     }
 
@@ -1666,9 +1661,11 @@  dpctl_unixctl_register(void)
     const struct dpctl_command *p;
 
     for (p = all_commands; p->name != NULL; p++) {
-        char *cmd_name = xasprintf("dpctl/%s", p->name);
-        unixctl_command_register(cmd_name, "", p->min_args, p->max_args,
-                                 dpctl_unixctl_handler, p->handler);
-        free(cmd_name);
+        if (strcmp(p->name, "help")) {
+            char *cmd_name = xasprintf("dpctl/%s", p->name);
+            unixctl_command_register(cmd_name, "", p->min_args, p->max_args,
+                                     dpctl_unixctl_handler, p->handler);
+            free(cmd_name);
+        }
     }
 }