[Qemu-devel] [PATCH v2] Fix linux-user crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.

Kan Li posted 1 patch 7 years ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20181024201303.114-1-likan_999.student@sina.com
Test docker-clang@ubuntu passed
Test checkpatch failed
Test asan passed
Test docker-mingw@fedora failed
Test docker-quick@centos7 passed
linux-user/syscall.c | 55 ++++++++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 25 deletions(-)
[Qemu-devel] [PATCH v2] Fix linux-user crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.
Posted by Kan Li 7 years ago

Summary:
This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
It is valid for ifc_buf to be NULL according to
http://man7.org/linux/man-pages/man7/netdevice.7.html.

Signed-off-by: Kan Li <likan_999.student@sina.com>
---
 linux-user/syscall.c | 55 ++++++++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 15b03e17b9..2453778cfd 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4138,28 +4138,32 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
     unlock_user(argptr, arg, 0);
 
     host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
-    target_ifc_len = host_ifconf->ifc_len;
     target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
-
     target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
-    nb_ifreq = target_ifc_len / target_ifreq_size;
-    host_ifc_len = nb_ifreq * sizeof(struct ifreq);
 
-    outbufsz = sizeof(*host_ifconf) + host_ifc_len;
-    if (outbufsz > MAX_STRUCT_SIZE) {
-        /* We can't fit all the extents into the fixed size buffer.
-         * Allocate one that is large enough and use it instead.
-         */
-        host_ifconf = malloc(outbufsz);
-        if (!host_ifconf) {
-            return -TARGET_ENOMEM;
+    if (target_ifc_buf != 0) {
+        target_ifc_len = host_ifconf->ifc_len;
+        nb_ifreq = target_ifc_len / target_ifreq_size;
+        host_ifc_len = nb_ifreq * sizeof(struct ifreq);
+
+        outbufsz = sizeof(*host_ifconf) + host_ifc_len;
+        if (outbufsz > MAX_STRUCT_SIZE) {
+            /* We can't fit all the extents into the fixed size buffer.
+             * Allocate one that is large enough and use it instead.
+             */
+            host_ifconf = malloc(outbufsz);
+            if (!host_ifconf) {
+                return -TARGET_ENOMEM;
+            }
+            memcpy(host_ifconf, buf_temp, sizeof(*host_ifconf));
+            free_buf = 1;
         }
-        memcpy(host_ifconf, buf_temp, sizeof(*host_ifconf));
-        free_buf = 1;
+        host_ifc_buf = (char*)host_ifconf + sizeof(*host_ifconf);
+
+        host_ifconf->ifc_len = host_ifc_len;
+    } else {
+      host_ifc_buf = NULL;
     }
-    host_ifc_buf = (char*)host_ifconf + sizeof(*host_ifconf);
-
-    host_ifconf->ifc_len = host_ifc_len;
     host_ifconf->ifc_buf = host_ifc_buf;
 
     ret = get_errno(safe_ioctl(fd, ie->host_cmd, host_ifconf));
@@ -4182,15 +4186,16 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
         thunk_convert(argptr, host_ifconf, arg_type, THUNK_TARGET);
         unlock_user(argptr, arg, target_size);
 
-	/* copy ifreq[] to target user */
-
-        argptr = lock_user(VERIFY_WRITE, target_ifc_buf, target_ifc_len, 0);
-        for (i = 0; i < nb_ifreq ; i++) {
-            thunk_convert(argptr + i * target_ifreq_size,
-                          host_ifc_buf + i * sizeof(struct ifreq),
-                          ifreq_arg_type, THUNK_TARGET);
+        if (target_ifc_buf != 0) {
+            /* copy ifreq[] to target user */
+            argptr = lock_user(VERIFY_WRITE, target_ifc_buf, target_ifc_len, 0);
+            for (i = 0; i < nb_ifreq ; i++) {
+                thunk_convert(argptr + i * target_ifreq_size,
+                              host_ifc_buf + i * sizeof(struct ifreq),
+                              ifreq_arg_type, THUNK_TARGET);
+            }
+            unlock_user(argptr, target_ifc_buf, target_ifc_len);
         }
-        unlock_user(argptr, target_ifc_buf, target_ifc_len);
     }
 
     if (free_buf) {
-- 
2.14.5



Re: [Qemu-devel] [PATCH v2] Fix linux-user crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.
Posted by Laurent Vivier 7 years ago
On 24/10/2018 21:13, Kan Li wrote:
> Summary:
> This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
> It is valid for ifc_buf to be NULL according to
> http://man7.org/linux/man-pages/man7/netdevice.7.html.
> 
> Signed-off-by: Kan Li <likan_999.student@sina.com>
> ---
>  linux-user/syscall.c | 55 ++++++++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 25 deletions(-)
> 

Reviewed-by: Laurent Vivier <lvivier@redhat.com>



Re: [Qemu-devel] [PATCH v2] Fix linux-user crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.
Posted by Peter Maydell 6 years, 9 months ago
On Wed, 24 Oct 2018 at 22:05, Laurent Vivier <laurent@vivier.eu> wrote:
>
> On 24/10/2018 21:13, Kan Li wrote:
> > Summary:
> > This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
> > It is valid for ifc_buf to be NULL according to
> > http://man7.org/linux/man-pages/man7/netdevice.7.html.
> >
> > Signed-off-by: Kan Li <likan_999.student@sina.com>
> > ---
> >  linux-user/syscall.c | 55 ++++++++++++++++++++++++++++------------------------
> >  1 file changed, 30 insertions(+), 25 deletions(-)
> >
>
> Reviewed-by: Laurent Vivier <lvivier@redhat.com>

Hi Laurent -- did this patch get lost? It doesn't seem to have
made it into master.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] Fix linux-user crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.
Posted by Laurent Vivier 6 years, 9 months ago
On 18/01/2019 18:09, Peter Maydell wrote:
> On Wed, 24 Oct 2018 at 22:05, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>> On 24/10/2018 21:13, Kan Li wrote:
>>> Summary:
>>> This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
>>> It is valid for ifc_buf to be NULL according to
>>> http://man7.org/linux/man-pages/man7/netdevice.7.html.
>>>
>>> Signed-off-by: Kan Li <likan_999.student@sina.com>
>>> ---
>>>   linux-user/syscall.c | 55 ++++++++++++++++++++++++++++------------------------
>>>   1 file changed, 30 insertions(+), 25 deletions(-)
>>>
>>
>> Reviewed-by: Laurent Vivier <lvivier@redhat.com>
> 
> Hi Laurent -- did this patch get lost? It doesn't seem to have
> made it into master.

Yes, thank you Peter.

It is applied now to my linux-user-for-4.0 branch.

Laurent