[libvirt] [PATCH] phyp: Fixing possible buffer overflow

[Laine Stump]

On 07/15/2010 06:37 PM, Eduardo Otubo wrote:
> The line src/phyp/phyp_driver.c:427 was crashing by buffer overflow
> if the return of the command wasn't <=10. The highest number for a
> LPAR ID is 256 per machine, no need to allocate 10 bytes for it. So,
> adjusting the correct size (+1 byte for the '\n') and checking for 
> errors.

This analysis doesn't make sense to me. On the one hand, you say that
the function was crashing if the return from the command wasn't <= 10
bytes long (implying that it sometimes was longer), on the other hand,
you say that part of the solution is to change the code to only handle
4 bytes (you forgot about the trailing NULL there, BTW - should have
been 3), because you'll never get more than that.

Also, your patch doesn't account for the possibility of nids > 1. I
don't know anything about the output of lssyscfg other than what I
learned from google just now, but it looks like you could end up with
multiple lines of output (and this function was certainly written to
at least attempt to parse multiple lines, ie multiple ids). Since you
error out if the entire string returned from the exec (ret) is > 4
bytes long (again, 3 was what you should have gone for, since you have
a 4 byte array and need to reserve one for the NULL), you would fail
if there was more than one value returned. You should instead be
making sure that *each individual line* is no longer than the limit.

Finally, you didn't change the length arg for the 2nd memset that's
inside the while loop, thus guaranteeing that you would overwrite 6
bytes of stack every time this function is called.


Of course, since virStrToLong_i will stop at the \n ending each line,
and not return an error since you're passing an endptr to it, why not
just do the conversion directly from the original string instead? That
way you wouldn't need to worry about all the memsets, checking for
overflows, etc, and you can use the pointer returned from the strtol
as the start of the next line, thus eliminating the overly complicated
character copy loop.

Here's a stab at doing it this way. I haven't even compiled it, but
you can give it a try and see if it solves your problem.


---
 src/phyp/phyp_driver.c |   45 ++++++++++++++++-----------------------------
 1 files changed, 16 insertions(+), 29 deletions(-)

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index ee1e21b..b6e4c5a 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -380,12 +380,10 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
     int system_type = phyp_driver->system_type;
     char *managed_system = phyp_driver->managed_system;
     int exit_status = 0;
-    int got = 0;
-    char *char_ptr;
-    unsigned int i = 0, j = 0;
-    char id_c[10];
+    int got = -1;
     char *cmd = NULL;
     char *ret = NULL;
+    char *line, *next_line;
     const char *state;
     virBuffer buf = VIR_BUFFER_INITIALIZER;
 
@@ -394,8 +392,6 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
     else
         state = " ";
 
-    memset(id_c, 0, 10);
-
     virBufferAddLit(&buf, "lssyscfg -r lpar");
     if (system_type == HMC)
         virBufferVSprintf(&buf, " -m %s", managed_system);
@@ -410,37 +406,28 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, int nids,
 
     ret = phypExec(session, cmd, &exit_status, conn);
 
-    /* I need to parse the textual return in order to get the ret */
     if (exit_status < 0 || ret == NULL)
         goto err;
-    else {
-        while (got < nids) {
-            if (ret[i] == '\0')
-                break;
-            else if (ret[i] == '\n') {
-                if (virStrToLong_i(id_c, &char_ptr, 10, &ids[got]) == -1) {
-                    VIR_ERROR(_("Cannot parse number from '%s'"), id_c);
-                    goto err;
-                }
-                memset(id_c, 0, 10);
-                j = 0;
-                got++;
-            } else {
-                id_c[j] = ret[i];
-                j++;
-            }
-            i++;
+
+    /* I need to parse the textual return in order to get the ids */
+    line = ret;
+    got = 0;
+    while (*line && got < nids) {
+        if (virStrToLong_i(line, &next_line, 10, &ids[got]) == -1) {
+            VIR_ERROR(_("Cannot parse number from '%s'"), line);
+            got = -1;
+            goto err;
         }
+        got++;
+        line = next_line;
+        while (*line == '\n')
+            line++; /* skip \n */
     }
 
-    VIR_FREE(cmd);
-    VIR_FREE(ret);
-    return got;
-
   err:
     VIR_FREE(cmd);
     VIR_FREE(ret);
-    return -1;
+    return got;
 }
 
 static int
-- 
1.7.1.1