[Qemu-devel] [PATCH 0/3] More netlink fixes

[Qemu-devel] [PATCH 0/3] More netlink fixes

[Laurent Vivier]

This series allows to run "apt-get update" or "dnf update"

Access to internet is hanging because glibc checks the netlink
sequence number when it scans for the local IP addresses,
and this number is corrupted. This is fixed by patch 2.

Once we have fixed this, passwd is not working anymore (audit netlink),
because data are not translated while using recvfrom(). This is fixed by
patch 3. Patch 1 is a pre-requesite for patch 3.

I've tested "apt-get update" and "passwd" with qemu-s390x (big-endian/64bit)
,qemu-m68k (big-endian/32bit, qemu-ppc64le (little-endian/64bit) on x86_64
(little-endian/64bit).

Laurent Vivier (3):
  linux-user: fd_trans_*_data() returns the length
  linux-user: fix netlink memory corruption
  linux-user: add fd_trans helper in do_recvfrom()

 linux-user/syscall.c | 71 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 59 insertions(+), 12 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/3] linux-user: fd_trans_*_data() returns the length

[Laurent Vivier]

fd_trans_target_to_host_data() and fd_trans_host_to_target_data() must
return the length of processed data.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0082762..9a5cd26 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2821,12 +2821,26 @@ static TargetFdTrans target_packet_trans = {
 #ifdef CONFIG_RTNETLINK
 static abi_long netlink_route_target_to_host(void *buf, size_t len)
 {
-    return target_to_host_nlmsg_route(buf, len);
+    abi_long ret;
+
+    ret = target_to_host_nlmsg_route(buf, len);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return len;
 }
 
 static abi_long netlink_route_host_to_target(void *buf, size_t len)
 {
-    return host_to_target_nlmsg_route(buf, len);
+    abi_long ret;
+
+    ret = host_to_target_nlmsg_route(buf, len);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return len;
 }
 
 static TargetFdTrans target_netlink_route_trans = {
@@ -2837,12 +2851,26 @@ static TargetFdTrans target_netlink_route_trans = {
 
 static abi_long netlink_audit_target_to_host(void *buf, size_t len)
 {
-    return target_to_host_nlmsg_audit(buf, len);
+    abi_long ret;
+
+    ret = target_to_host_nlmsg_audit(buf, len);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return len;
 }
 
 static abi_long netlink_audit_host_to_target(void *buf, size_t len)
 {
-    return host_to_target_nlmsg_audit(buf, len);
+    abi_long ret;
+
+    ret = host_to_target_nlmsg_audit(buf, len);
+    if (ret < 0) {
+        return ret;
+    }
+
+    return len;
 }
 
 static TargetFdTrans target_netlink_audit_trans = {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/3] linux-user: fd_trans_*_data() returns the length

[Laurent Vivier]

Ping?

Laurent

Le 21/06/2016 à 19:51, Laurent Vivier a écrit :
> fd_trans_target_to_host_data() and fd_trans_host_to_target_data() must
> return the length of processed data.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/syscall.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 0082762..9a5cd26 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2821,12 +2821,26 @@ static TargetFdTrans target_packet_trans = {
>  #ifdef CONFIG_RTNETLINK
>  static abi_long netlink_route_target_to_host(void *buf, size_t len)
>  {
> -    return target_to_host_nlmsg_route(buf, len);
> +    abi_long ret;
> +
> +    ret = target_to_host_nlmsg_route(buf, len);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return len;
>  }
>  
>  static abi_long netlink_route_host_to_target(void *buf, size_t len)
>  {
> -    return host_to_target_nlmsg_route(buf, len);
> +    abi_long ret;
> +
> +    ret = host_to_target_nlmsg_route(buf, len);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return len;
>  }
>  
>  static TargetFdTrans target_netlink_route_trans = {
> @@ -2837,12 +2851,26 @@ static TargetFdTrans target_netlink_route_trans = {
>  
>  static abi_long netlink_audit_target_to_host(void *buf, size_t len)
>  {
> -    return target_to_host_nlmsg_audit(buf, len);
> +    abi_long ret;
> +
> +    ret = target_to_host_nlmsg_audit(buf, len);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return len;
>  }
>  
>  static abi_long netlink_audit_host_to_target(void *buf, size_t len)
>  {
> -    return host_to_target_nlmsg_audit(buf, len);
> +    abi_long ret;
> +
> +    ret = host_to_target_nlmsg_audit(buf, len);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +
> +    return len;
>  }
>  
>  static TargetFdTrans target_netlink_audit_trans = {
> 

Re: [Qemu-devel] [PATCH 1/3] linux-user: fd_trans_*_data() returns the length

[Riku Voipio]

Hi,

I've merged the netlink patches the que[1]. I'm waiting to see if
we'll get some fixup patches to the fairly substantial changes
linux-user has seen in this cycle.

Riku

[1] https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream
On Tue, Jun 28, 2016 at 06:50:20PM +0200, Laurent Vivier wrote:
> Ping?
> 
> Laurent
> 
> Le 21/06/2016 à 19:51, Laurent Vivier a écrit :
> > fd_trans_target_to_host_data() and fd_trans_host_to_target_data() must
> > return the length of processed data.
> > 
> > Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> >  linux-user/syscall.c | 36 ++++++++++++++++++++++++++++++++----
> >  1 file changed, 32 insertions(+), 4 deletions(-)
> > 
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 0082762..9a5cd26 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -2821,12 +2821,26 @@ static TargetFdTrans target_packet_trans = {
> >  #ifdef CONFIG_RTNETLINK
> >  static abi_long netlink_route_target_to_host(void *buf, size_t len)
> >  {
> > -    return target_to_host_nlmsg_route(buf, len);
> > +    abi_long ret;
> > +
> > +    ret = target_to_host_nlmsg_route(buf, len);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    return len;
> >  }
> >  
> >  static abi_long netlink_route_host_to_target(void *buf, size_t len)
> >  {
> > -    return host_to_target_nlmsg_route(buf, len);
> > +    abi_long ret;
> > +
> > +    ret = host_to_target_nlmsg_route(buf, len);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    return len;
> >  }
> >  
> >  static TargetFdTrans target_netlink_route_trans = {
> > @@ -2837,12 +2851,26 @@ static TargetFdTrans target_netlink_route_trans = {
> >  
> >  static abi_long netlink_audit_target_to_host(void *buf, size_t len)
> >  {
> > -    return target_to_host_nlmsg_audit(buf, len);
> > +    abi_long ret;
> > +
> > +    ret = target_to_host_nlmsg_audit(buf, len);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    return len;
> >  }
> >  
> >  static abi_long netlink_audit_host_to_target(void *buf, size_t len)
> >  {
> > -    return host_to_target_nlmsg_audit(buf, len);
> > +    abi_long ret;
> > +
> > +    ret = host_to_target_nlmsg_audit(buf, len);
> > +    if (ret < 0) {
> > +        return ret;
> > +    }
> > +
> > +    return len;
> >  }
> >  
> >  static TargetFdTrans target_netlink_audit_trans = {
> > 

Re: [Qemu-devel] [PATCH 1/3] linux-user: fd_trans_*_data() returns the length

[Laurent Vivier]

Le 30/06/2016 à 09:52, Riku Voipio a écrit :
> 
> Hi,
> 
> I've merged the netlink patches the que[1]. I'm waiting to see if
> we'll get some fixup patches to the fairly substantial changes
> linux-user has seen in this cycle.

Perfect.

Thanks,
Laurent

> 
> Riku
> 
> [1] https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream
> On Tue, Jun 28, 2016 at 06:50:20PM +0200, Laurent Vivier wrote:
>> Ping?
>>
>> Laurent
>>
>> Le 21/06/2016 à 19:51, Laurent Vivier a écrit :
>>> fd_trans_target_to_host_data() and fd_trans_host_to_target_data() must
>>> return the length of processed data.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>  linux-user/syscall.c | 36 ++++++++++++++++++++++++++++++++----
>>>  1 file changed, 32 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 0082762..9a5cd26 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -2821,12 +2821,26 @@ static TargetFdTrans target_packet_trans = {
>>>  #ifdef CONFIG_RTNETLINK
>>>  static abi_long netlink_route_target_to_host(void *buf, size_t len)
>>>  {
>>> -    return target_to_host_nlmsg_route(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = target_to_host_nlmsg_route(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static abi_long netlink_route_host_to_target(void *buf, size_t len)
>>>  {
>>> -    return host_to_target_nlmsg_route(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = host_to_target_nlmsg_route(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static TargetFdTrans target_netlink_route_trans = {
>>> @@ -2837,12 +2851,26 @@ static TargetFdTrans target_netlink_route_trans = {
>>>  
>>>  static abi_long netlink_audit_target_to_host(void *buf, size_t len)
>>>  {
>>> -    return target_to_host_nlmsg_audit(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = target_to_host_nlmsg_audit(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static abi_long netlink_audit_host_to_target(void *buf, size_t len)
>>>  {
>>> -    return host_to_target_nlmsg_audit(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = host_to_target_nlmsg_audit(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static TargetFdTrans target_netlink_audit_trans = {
>>>

Re: [Qemu-devel] [PATCH 1/3] linux-user: fd_trans_*_data() returns the length

[Laurent Vivier]

Le 30/06/2016 à 09:52, Riku Voipio a écrit :
> 
> Hi,
> 
> I've merged the netlink patches the que[1]. I'm waiting to see if
> we'll get some fixup patches to the fairly substantial changes
> linux-user has seen in this cycle.
> 
> Riku
> 
> [1] https://git.linaro.org/people/riku.voipio/qemu.git/shortlog/refs/heads/linux-user-for-upstream

Ping?

Laurent

> On Tue, Jun 28, 2016 at 06:50:20PM +0200, Laurent Vivier wrote:
>> Ping?
>>
>> Laurent
>>
>> Le 21/06/2016 à 19:51, Laurent Vivier a écrit :
>>> fd_trans_target_to_host_data() and fd_trans_host_to_target_data() must
>>> return the length of processed data.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>  linux-user/syscall.c | 36 ++++++++++++++++++++++++++++++++----
>>>  1 file changed, 32 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>>> index 0082762..9a5cd26 100644
>>> --- a/linux-user/syscall.c
>>> +++ b/linux-user/syscall.c
>>> @@ -2821,12 +2821,26 @@ static TargetFdTrans target_packet_trans = {
>>>  #ifdef CONFIG_RTNETLINK
>>>  static abi_long netlink_route_target_to_host(void *buf, size_t len)
>>>  {
>>> -    return target_to_host_nlmsg_route(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = target_to_host_nlmsg_route(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static abi_long netlink_route_host_to_target(void *buf, size_t len)
>>>  {
>>> -    return host_to_target_nlmsg_route(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = host_to_target_nlmsg_route(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static TargetFdTrans target_netlink_route_trans = {
>>> @@ -2837,12 +2851,26 @@ static TargetFdTrans target_netlink_route_trans = {
>>>  
>>>  static abi_long netlink_audit_target_to_host(void *buf, size_t len)
>>>  {
>>> -    return target_to_host_nlmsg_audit(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = target_to_host_nlmsg_audit(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static abi_long netlink_audit_host_to_target(void *buf, size_t len)
>>>  {
>>> -    return host_to_target_nlmsg_audit(buf, len);
>>> +    abi_long ret;
>>> +
>>> +    ret = host_to_target_nlmsg_audit(buf, len);
>>> +    if (ret < 0) {
>>> +        return ret;
>>> +    }
>>> +
>>> +    return len;
>>>  }
>>>  
>>>  static TargetFdTrans target_netlink_audit_trans = {
>>>

[Qemu-devel] [PATCH 2/3] linux-user: fix netlink memory corruption

[Laurent Vivier]

Netlink is byte-swapping data in the guest memory (it's bad).

It's ok when the data come from the host as they are generated by the
host.

But it doesn't work when data come from the guest: the guest can
try to reuse these data whereas they have been byte-swapped.

This is what happens in glibc:

glibc generates a sequence number in nlh.nlmsg_seq and calls
sendto() with this nlh. In sendto(), we byte-swap nlmsg.seq.

Later, after the recvmsg(), glibc compares nlh.nlmsg_seq with
sequence number given in return, and of course it fails (hangs),
because nlh.nlmsg_seq is not valid anymore.

The involved code in glibc is:

sysdeps/unix/sysv/linux/check_pf.c:make_request()
...
  req.nlh.nlmsg_seq = time (NULL);
...
  if (TEMP_FAILURE_RETRY (__sendto (fd, (void *) &req, sizeof (req), 0,
                                    (struct sockaddr *) &nladdr,
                                    sizeof (nladdr))) < 0)
<here req.nlh.nlmsg_seq has been byte-swapped>
...
  do
    {
...
      ssize_t read_len = TEMP_FAILURE_RETRY (__recvmsg (fd, &msg, 0));
...
      struct nlmsghdr *nlmh;
      for (nlmh = (struct nlmsghdr *) buf;
           NLMSG_OK (nlmh, (size_t) read_len);
           nlmh = (struct nlmsghdr *) NLMSG_NEXT (nlmh, read_len))
        {
<we compare nlmh->nlmsg_seq with corrupted req.nlh.nlmsg_seq>
          if (nladdr.nl_pid != 0 || (pid_t) nlmh->nlmsg_pid != pid
              || nlmh->nlmsg_seq != req.nlh.nlmsg_seq)
            continue;
...
          else if (nlmh->nlmsg_type == NLMSG_DONE)
            /* We found the end, leave the loop.  */
            done = true;
        }
    }
  while (! done);

As we have a continue on "nlmh->nlmsg_seq != req.nlh.nlmsg_seq",
"done" cannot be set to "true" and we have an infinite loop.

It's why commands like "apt-get update" or "dnf update hangs".

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 32 ++++++++++++++++++++++++--------
 1 file changed, 24 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 9a5cd26..fdc884f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3012,13 +3012,22 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
 
     if (send) {
         if (fd_trans_target_to_host_data(fd)) {
-            ret = fd_trans_target_to_host_data(fd)(msg.msg_iov->iov_base,
+            void *host_msg;
+
+            host_msg = g_malloc(msg.msg_iov->iov_len);
+            memcpy(host_msg, msg.msg_iov->iov_base, msg.msg_iov->iov_len);
+            ret = fd_trans_target_to_host_data(fd)(host_msg,
                                                    msg.msg_iov->iov_len);
+            if (ret >= 0) {
+                msg.msg_iov->iov_base = host_msg;
+                ret = get_errno(safe_sendmsg(fd, &msg, flags));
+            }
+            g_free(host_msg);
         } else {
             ret = target_to_host_cmsg(&msg, msgp);
-        }
-        if (ret == 0) {
-            ret = get_errno(safe_sendmsg(fd, &msg, flags));
+            if (ret == 0) {
+                ret = get_errno(safe_sendmsg(fd, &msg, flags));
+            }
         }
     } else {
         ret = get_errno(safe_recvmsg(fd, &msg, flags));
@@ -3234,6 +3243,7 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags,
 {
     void *addr;
     void *host_msg;
+    void *copy_msg = NULL;
     abi_long ret;
 
     if ((int)addrlen < 0) {
@@ -3244,23 +3254,29 @@ static abi_long do_sendto(int fd, abi_ulong msg, size_t len, int flags,
     if (!host_msg)
         return -TARGET_EFAULT;
     if (fd_trans_target_to_host_data(fd)) {
+        copy_msg = host_msg;
+        host_msg = g_malloc(len);
+        memcpy(host_msg, copy_msg, len);
         ret = fd_trans_target_to_host_data(fd)(host_msg, len);
         if (ret < 0) {
-            unlock_user(host_msg, msg, 0);
-            return ret;
+            goto fail;
         }
     }
     if (target_addr) {
         addr = alloca(addrlen+1);
         ret = target_to_host_sockaddr(fd, addr, target_addr, addrlen);
         if (ret) {
-            unlock_user(host_msg, msg, 0);
-            return ret;
+            goto fail;
         }
         ret = get_errno(safe_sendto(fd, host_msg, len, flags, addr, addrlen));
     } else {
         ret = get_errno(safe_sendto(fd, host_msg, len, flags, NULL, 0));
     }
+fail:
+    if (copy_msg) {
+        g_free(host_msg);
+        host_msg = copy_msg;
+    }
     unlock_user(host_msg, msg, 0);
     return ret;
 }
-- 
2.5.5

[Qemu-devel] [PATCH 3/3] linux-user: add fd_trans helper in do_recvfrom()

[Laurent Vivier]

Fix passwd using netlink audit.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index fdc884f..125a3fd 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3311,6 +3311,9 @@ static abi_long do_recvfrom(int fd, abi_ulong msg, size_t len, int flags,
         ret = get_errno(safe_recvfrom(fd, host_msg, len, flags, NULL, 0));
     }
     if (!is_error(ret)) {
+        if (fd_trans_host_to_target_data(fd)) {
+            ret = fd_trans_host_to_target_data(fd)(host_msg, ret);
+        }
         if (target_addr) {
             host_to_target_sockaddr(target_addr, addr, addrlen);
             if (put_user_u32(addrlen, target_addrlen)) {
-- 
2.5.5

Re: [Qemu-devel] [PATCH 0/3] More netlink fixes

[Peter Maydell]

On 21 June 2016 at 18:51, Laurent Vivier <laurent@vivier.eu> wrote:
> This series allows to run "apt-get update" or "dnf update"
>
> Access to internet is hanging because glibc checks the netlink
> sequence number when it scans for the local IP addresses,
> and this number is corrupted. This is fixed by patch 2.
>
> Once we have fixed this, passwd is not working anymore (audit netlink),
> because data are not translated while using recvfrom(). This is fixed by
> patch 3. Patch 1 is a pre-requesite for patch 3.
>
> I've tested "apt-get update" and "passwd" with qemu-s390x (big-endian/64bit)
> ,qemu-m68k (big-endian/32bit, qemu-ppc64le (little-endian/64bit) on x86_64
> (little-endian/64bit).

Are these fixes for regressions? "apt-get update" has worked
for me for ages.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 0/3] More netlink fixes

[Laurent Vivier]

Le 21/06/2016 à 19:55, Peter Maydell a écrit :
> On 21 June 2016 at 18:51, Laurent Vivier <laurent@vivier.eu> wrote:
>> This series allows to run "apt-get update" or "dnf update"
>>
>> Access to internet is hanging because glibc checks the netlink
>> sequence number when it scans for the local IP addresses,
>> and this number is corrupted. This is fixed by patch 2.
>>
>> Once we have fixed this, passwd is not working anymore (audit netlink),
>> because data are not translated while using recvfrom(). This is fixed by
>> patch 3. Patch 1 is a pre-requesite for patch 3.
>>
>> I've tested "apt-get update" and "passwd" with qemu-s390x (big-endian/64bit)
>> ,qemu-m68k (big-endian/32bit, qemu-ppc64le (little-endian/64bit) on x86_64
>> (little-endian/64bit).
> 
> Are these fixes for regressions? "apt-get update" has worked
> for me for ages.

Yes.

When netlink is not available, glibc doesn't use it and "apt-get
update", "wget", "dnf update", ... work fine.

But when netlink is available, glibc uses it and we meet this bug: they
hang.

But it happens only if endianness differs between host and guest.

Laurent