[Qemu-devel] [PATCH] qga: Fix possible freed memory accessing

[Qemu-devel] [PATCH] qga: Fix possible freed memory accessing

[zhanghailiang]

If readdir_r fails, error_setg_errno will reference the freed
pointer *dirpath*.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 qga/commands-posix.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/qga/commands-posix.c b/qga/commands-posix.c
index 7eed7f4..3082eae 100644
--- a/qga/commands-posix.c
+++ b/qga/commands-posix.c
@@ -965,7 +965,6 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
         g_free(dirpath);
         return;
     }
-    g_free(dirpath);
 
     for (;;) {
         if (readdir_r(dir, &entry, &result) != 0) {
@@ -977,10 +976,12 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
         }
 
         if (entry.d_type == DT_LNK) {
+            char *path;
+
             g_debug(" slave device '%s'", entry.d_name);
-            dirpath = g_strdup_printf("%s/slaves/%s", syspath, entry.d_name);
-            build_guest_fsinfo_for_device(dirpath, fs, errp);
-            g_free(dirpath);
+            path = g_strdup_printf("%s/slaves/%s", syspath, entry.d_name);
+            build_guest_fsinfo_for_device(path, fs, errp);
+            g_free(path);
 
             if (*errp) {
                 break;
@@ -988,6 +989,7 @@ static void build_guest_fsinfo_for_virtual_device(char const *syspath,
         }
     }
 
+    g_free(dirpath);
     closedir(dir);
 }
 
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] qga: Fix possible freed memory accessing

[Markus Armbruster]

zhanghailiang <zhang.zhanghailiang@huawei.com> writes:

> If readdir_r fails, error_setg_errno will reference the freed
> pointer *dirpath*.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] qga: Fix possible freed memory accessing

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

On 09/17/2014 09:33 PM, zhanghailiang wrote:
> If readdir_r fails, error_setg_errno will reference the freed
> pointer *dirpath*.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  qga/commands-posix.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

>      for (;;) {
>          if (readdir_r(dir, &entry, &result) != 0) {

Eww.  We're using readdir_r?  That's an inherently broken interface,
which can risk buffer overflow.  readdir should be preferred.

http://austingroupbugs.net/view.php?id=696

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH] qga: Fix possible freed memory accessing

[zhanghailiang]

On 2014/9/18 20:17, Eric Blake wrote:
> On 09/17/2014 09:33 PM, zhanghailiang wrote:
>> If readdir_r fails, error_setg_errno will reference the freed
>> pointer *dirpath*.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>   qga/commands-posix.c | 10 ++++++----
>>   1 file changed, 6 insertions(+), 4 deletions(-)
>
>>       for (;;) {
>>           if (readdir_r(dir, &entry, &result) != 0) {
>
> Eww.  We're using readdir_r?  That's an inherently broken interface,
> which can risk buffer overflow.  readdir should be preferred.
>
> http://austingroupbugs.net/view.php?id=696
>

Yes, it is! Should i fix it in this patch together?;)

Thanks,
zhanghailiang

Re: [Qemu-devel] [PATCH] qga: Fix possible freed memory accessing

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

On 09/18/2014 06:42 AM, zhanghailiang wrote:
> On 2014/9/18 20:17, Eric Blake wrote:
>> On 09/17/2014 09:33 PM, zhanghailiang wrote:
>>> If readdir_r fails, error_setg_errno will reference the freed
>>> pointer *dirpath*.
>>>
>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>> ---
>>>   qga/commands-posix.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>
>>>       for (;;) {
>>>           if (readdir_r(dir, &entry, &result) != 0) {
>>
>> Eww.  We're using readdir_r?  That's an inherently broken interface,
>> which can risk buffer overflow.  readdir should be preferred.
>>
>> http://austingroupbugs.net/view.php?id=696
>>
> 
> Yes, it is! Should i fix it in this patch together?;)

Switching to readdir would be welcome, and would probably be enough of a
rewrite that it would also fix the use-after-free without trying to
break it into two patches.  You're welcome to try that as a v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]