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

[Eduardo Otubo]

On 07/15/2010 11:01 PM, Laine Stump wrote:
> 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.

I'm sorry. My bad here. The lssyscfg (with the correct parameters and 
greps) will return something like this:

5
4
6
7
10
1
143

These are the LPAR IDs. Each line cannot be bigger than 3 characters (+1 
for '\n'). So, I just allocate 4 bytes to hold these values, one at a 
time and memset it with zeros. If there's an error, there will be a 
single line with the error message, like this one:

"HSCL8018 The managed system rico was not found."

IMO the best choice is to loop in the result and if I can't find a '\n' 
after 3 character, I return an error. My mistake was to check the entire 
string returned from exec. My bad, sorry.

I think this patch is much clearer on what the problem is.

Thanks for the comments!

---
  src/phyp/phyp_driver.c |    6 +++---
  1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c
index ee1e21b..0c50c9a 100644
--- a/src/phyp/phyp_driver.c
+++ b/src/phyp/phyp_driver.c
@@ -383,7 +383,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, 
int nids,
      int got = 0;
      char *char_ptr;
      unsigned int i = 0, j = 0;
-    char id_c[10];
+    char id_c[4];
      char *cmd = NULL;
      char *ret = NULL;
      const char *state;
@@ -394,7 +394,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, 
int nids,
      else
          state = " ";

-    memset(id_c, 0, 10);
+    memset(id_c, 0, 4);

      virBufferAddLit(&buf, "lssyscfg -r lpar");
      if (system_type == HMC)
@@ -414,7 +414,7 @@ phypListDomainsGeneric(virConnectPtr conn, int *ids, 
int nids,
      if (exit_status < 0 || ret == NULL)
          goto err;
      else {
-        while (got < nids) {
+        while (got < nids && i <= 4) {
              if (ret[i] == '\0')
                  break;
              else if (ret[i] == '\n') {
--
1.7.0.4