[Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Chen Gang]

strncat() will append additional '\0' to destination buffer, so need
additional 1 byte for it, or may cause memory overflow, just like other
area within QEMU have done.

Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 target-ppc/kvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9c23c6b..66e7ce5 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1794,8 +1794,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
         return -1;
     }
 
-    strncat(buf, "/", sizeof(buf) - strlen(buf));
-    strncat(buf, propname, sizeof(buf) - strlen(buf));
+    strncat(buf, "/", sizeof(buf) - strlen(buf) - 1);
+    strncat(buf, propname, sizeof(buf) - strlen(buf) - 1);
 
     f = fopen(buf, "rb");
     if (!f) {
-- 
1.9.3

Re: [Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Alexander Graf]

On 13.10.14 16:36, Chen Gang wrote:
> strncat() will append additional '\0' to destination buffer, so need
> additional 1 byte for it, or may cause memory overflow, just like other
> area within QEMU have done.
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

I agree with this patch. However, the code is pretty ugly - I'm sure it
must've been me who wrote it :).

Could you please instead rewrite it to use g_strdup_printf() rather than
strncat()s? That way we resolve all string pitfalls automatically - and
this code is not the fast path, so doing an extra memory allocation is ok.


Alex

> ---
>  target-ppc/kvm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..66e7ce5 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1794,8 +1794,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>          return -1;
>      }
>  
> -    strncat(buf, "/", sizeof(buf) - strlen(buf));
> -    strncat(buf, propname, sizeof(buf) - strlen(buf));
> +    strncat(buf, "/", sizeof(buf) - strlen(buf) - 1);
> +    strncat(buf, propname, sizeof(buf) - strlen(buf) - 1);
>  
>      f = fopen(buf, "rb");
>      if (!f) {
> 

Re: [Qemu-devel] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Chen Gang]

On 10/13/14 22:47, Alexander Graf wrote:
> 
> Could you please instead rewrite it to use g_strdup_printf() rather than
> strncat()s? That way we resolve all string pitfalls automatically - and
> this code is not the fast path, so doing an extra memory allocation is ok.
>

I guess, it is a personal taste. For me, it may need additional variable
for g_strdup_printf(), and not save code lines, but *sprintf() is more
readable than str*cat().

The related code may like below (welcome any improvement for it):

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 66e7ce5..cea6a87 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1782,7 +1782,7 @@ static int kvmppc_find_cpu_dt(char *buf, int buf_len)
  * format) */
 static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
 {
-    char buf[PATH_MAX];
+    char buf[PATH_MAX], *tmp;
     union {
         uint32_t v32;
         uint64_t v64;
@@ -1794,10 +1794,10 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
         return -1;
     }
 
-    strncat(buf, "/", sizeof(buf) - strlen(buf) - 1);
-    strncat(buf, propname, sizeof(buf) - strlen(buf) - 1);
+    tmp = g_strdup_printf("%s/%s", buf, propname);
 
-    f = fopen(buf, "rb");
+    f = fopen(tmp, "rb");
+    g_free(buf);
     if (!f) {
         return -1;
     }

For me, it is really a personal taste, so if the maintainer feels the
diff above is OK, I shall send patch v2 for it within 2 days.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Michael Tokarev]

On 10/13/2014 06:47 PM, Alexander Graf wrote:
> On 13.10.14 16:36, Chen Gang wrote:
>> strncat() will append additional '\0' to destination buffer, so need
>> additional 1 byte for it, or may cause memory overflow, just like other
>> area within QEMU have done.
>>
>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> 
> I agree with this patch. However, the code is pretty ugly - I'm sure it
> must've been me who wrote it :).
> 
> Could you please instead rewrite it to use g_strdup_printf() rather than
> strncat()s? That way we resolve all string pitfalls automatically - and
> this code is not the fast path, so doing an extra memory allocation is ok.

I'd just use snprintf() like this:

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 9c23c6b..5eaa36c 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1794,8 +1794,7 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
         return -1;
     }

-    strncat(buf, "/", sizeof(buf) - strlen(buf));
-    strncat(buf, propname, sizeof(buf) - strlen(buf));
+    snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "/%s", propname);

     f = fopen(buf, "rb");
     if (!f) {

the buffer is of size PATH_MAX, and we're looking at /proc filesystem where
names should be rather short so we're extremly unlikely to hit this prob in
practice, there's no need to dynamically allocate a buffer for this stuff.

(Or alternatively there's asprintf(), but still I think it is overkill).

I can apply the above if everyone agrees.

Thanks,

/mjt

>>  target-ppc/kvm.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
>> index 9c23c6b..66e7ce5 100644
>> --- a/target-ppc/kvm.c
>> +++ b/target-ppc/kvm.c
>> @@ -1794,8 +1794,8 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>>          return -1;
>>      }
>>  
>> -    strncat(buf, "/", sizeof(buf) - strlen(buf));
>> -    strncat(buf, propname, sizeof(buf) - strlen(buf));
>> +    strncat(buf, "/", sizeof(buf) - strlen(buf) - 1);
>> +    strncat(buf, propname, sizeof(buf) - strlen(buf) - 1);
>>  
>>      f = fopen(buf, "rb");
>>      if (!f) {
>>
> 

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Alexander Graf]

> Am 24.10.2014 um 09:49 schrieb Michael Tokarev <mjt@tls.msk.ru>:
> 
>> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>>> On 13.10.14 16:36, Chen Gang wrote:
>>> strncat() will append additional '\0' to destination buffer, so need
>>> additional 1 byte for it, or may cause memory overflow, just like other
>>> area within QEMU have done.
>>> 
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>> 
>> I agree with this patch. However, the code is pretty ugly - I'm sure it
>> must've been me who wrote it :).
>> 
>> Could you please instead rewrite it to use g_strdup_printf() rather than
>> strncat()s? That way we resolve all string pitfalls automatically - and
>> this code is not the fast path, so doing an extra memory allocation is ok.
> 
> I'd just use snprintf() like this:
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1794,8 +1794,7 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>         return -1;
>     }
> 
> -    strncat(buf, "/", sizeof(buf) - strlen(buf));
> -    strncat(buf, propname, sizeof(buf) - strlen(buf));
> +    snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "/%s", propname);
> 
>     f = fopen(buf, "rb");
>     if (!f) {
> 
> the buffer is of size PATH_MAX, and we're looking at /proc filesystem where
> names should be rather short so we're extremly unlikely to hit this prob in
> practice, there's no need to dynamically allocate a buffer for this stuff.

I've already applied the fix to ppc-next.

> 
> (Or alternatively there's asprintf(), but still I think it is overkill).

That one isn't portable iirc.

> 
> I can apply the above if everyone agrees.

No worries, it's all over already ;).

Alex

Re: [Qemu-devel] [Qemu-trivial] [PATCH] target-ppc: kvm: Fix memory overflow issue about strncat()

[Chen Gang]

On 10/24/14 15:49, Michael Tokarev wrote:
> On 10/13/2014 06:47 PM, Alexander Graf wrote:
>> On 13.10.14 16:36, Chen Gang wrote:
>>> strncat() will append additional '\0' to destination buffer, so need
>>> additional 1 byte for it, or may cause memory overflow, just like other
>>> area within QEMU have done.
>>>
>>> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
>>
>> I agree with this patch. However, the code is pretty ugly - I'm sure it
>> must've been me who wrote it :).
>>
>> Could you please instead rewrite it to use g_strdup_printf() rather than
>> strncat()s? That way we resolve all string pitfalls automatically - and
>> this code is not the fast path, so doing an extra memory allocation is ok.
> 
> I'd just use snprintf() like this:
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..5eaa36c 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1794,8 +1794,7 @@ static uint64_t kvmppc_read_int_cpu_dt(const char *propname)
>          return -1;
>      }
> 
> -    strncat(buf, "/", sizeof(buf) - strlen(buf));
> -    strncat(buf, propname, sizeof(buf) - strlen(buf));
> +    snprintf(buf + strlen(buf), sizeof(buf) - strlen(buf), "/%s", propname);
> 
>      f = fopen(buf, "rb");
>      if (!f) {
> 
> the buffer is of size PATH_MAX, and we're looking at /proc filesystem where
> names should be rather short so we're extremly unlikely to hit this prob in
> practice, there's no need to dynamically allocate a buffer for this stuff.
> 
> (Or alternatively there's asprintf(), but still I think it is overkill).
> 
> I can apply the above if everyone agrees.
> 

For me, what you said is reasonable, although I am not sure whether the
patch v2 for g_strdup_printf() was already applied or not.

Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed