[Qemu-devel] [PATCH 0/2] linux-user: Fix length handling in cmsg conversions

[Qemu-devel] [PATCH 0/2] linux-user: Fix length handling in cmsg conversions

[Peter Maydell]

This patchset fixes some problems in conversions of cmsg structures
in target_to_host_cmsg() (used in send/recvmsg handling). Specifically:

     * we required the msg->msg_controllen to declare the buffer
       to have enough space for final trailing padding (we were
       checking against CMSG_SPACE), whereas the kernel does not
       require this, and common userspace code assumes this.
     * we weren't correctly handling the fact that the SO_TIMESTAMP
       payload may be larger for the target than the host
     * we weren't marking the messages with MSG_CTRUNC when we did
       need to truncate a message that wasn't truncated by the host,
       but were instead logging a QEMU message; since truncation is
       always the result of a guest giving us an insufficiently
       sized buffer, we should report it to the guest as the kernel
       does and don't log anything
     * we weren't handling the possibility of the host having a
       more restrictive alignment requirement for payload structs

The major visible issue I wanted to fix is that glibc's "try to talk
to nscd" code that it will run on startup will receive a cmsg with a
4 byte payload and only allocates 4 bytes for it, which was causing
us to do the wrong thing on architectures that need 8-alignment
(we dropped the cmsg and printed a diagnostic message).


Peter Maydell (2):
  linux-user: Fix length handling in host_to_target_cmsg
  linux-user: use __get_user and __put_user in cmsg conversions

 linux-user/syscall.c | 89 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 72 insertions(+), 17 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/2] linux-user: Fix length handling in host_to_target_cmsg

[Peter Maydell]

The previous code for handling payload length when converting
cmsg structures from host to target had a number of problems:
 * we required the msg->msg_controllen to declare the buffer
   to have enough space for final trailing padding (we were
   checking against CMSG_SPACE), whereas the kernel does not
   require this, and common userspace code assumes this. (In
   particular, glibc's "try to talk to nscd" code that it will
   run on startup will receive a cmsg with a 4 byte payload and
   only allocate 4 bytes for it, which was causing us to do
   the wrong thing on architectures that need 8-alignment.)
 * we weren't correctly handling the fact that the SO_TIMESTAMP
   payload may be larger for the target than the host
 * we weren't marking the messages with MSG_CTRUNC when we did
   need to truncate a message that wasn't truncated by the host,
   but were instead logging a QEMU message; since truncation is
   always the result of a guest giving us an insufficiently
   sized buffer, we should report it to the guest as the kernel
   does and don't log anything

Rewrite the parts of the function that deal with length to
fix these issues, and add a comment in target_to_host_cmsg
to explain why the overflow logging it does is a QEMU bug,
not a guest issue.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 61 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..3db6a90 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1202,6 +1202,15 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         space += CMSG_SPACE(len);
         if (space > msgh->msg_controllen) {
             space -= CMSG_SPACE(len);
+            /* This is a QEMU bug, since we allocated the payload
+             * area ourselves (unlike overflow in host-to-target
+             * conversion, which is just the guest giving us a buffer
+             * that's too small). It can't happen for the payload types
+             * we currently support; if it becomes an issue in future
+             * we would need to improve our allocation strategy to
+             * something more intelligent than "twice the size of the
+             * target buffer we're reading from".
+             */
             gemu_log("Host cmsg overflow\n");
             break;
         }
@@ -1267,11 +1276,16 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         void *target_data = TARGET_CMSG_DATA(target_cmsg);
 
         int len = cmsg->cmsg_len - CMSG_ALIGN(sizeof (struct cmsghdr));
+        int tgt_len, tgt_space;
 
-        space += TARGET_CMSG_SPACE(len);
-        if (space > msg_controllen) {
-            space -= TARGET_CMSG_SPACE(len);
-            gemu_log("Target cmsg overflow\n");
+        /* We never copy a half-header but may copy half-data;
+         * this is Linux's behaviour in put_cmsg(). Note that
+         * truncation here is a guest problem (which we report
+         * to the guest via the CTRUNC bit), unlike truncation
+         * in target_to_host_cmsg, which is a QEMU bug.
+         */
+        if (msg_controllen < sizeof(struct cmsghdr)) {
+            target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
             break;
         }
 
@@ -1281,8 +1295,35 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
             target_cmsg->cmsg_level = tswap32(cmsg->cmsg_level);
         }
         target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
-        target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(len));
 
+        tgt_len = TARGET_CMSG_LEN(len);
+
+        /* Payload types which need a different size of payload on
+         * the target must adjust tgt_len here.
+         */
+        switch (cmsg->cmsg_level) {
+        case SOL_SOCKET:
+            switch (cmsg->cmsg_type) {
+            case SO_TIMESTAMP:
+                tgt_len = sizeof(struct target_timeval);
+                break;
+            default:
+                break;
+            }
+        default:
+            break;
+        }
+
+        if (msg_controllen < tgt_len) {
+            target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
+            tgt_len = msg_controllen;
+        }
+
+        /* We must now copy-and-convert len bytes of payload
+         * into tgt_len bytes of destination space. Bear in mind
+         * that in both source and destination we may be dealing
+         * with a truncated value!
+         */
         switch (cmsg->cmsg_level) {
         case SOL_SOCKET:
             switch (cmsg->cmsg_type) {
@@ -1290,7 +1331,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
             {
                 int *fd = (int *)data;
                 int *target_fd = (int *)target_data;
-                int i, numfds = len / sizeof(int);
+                int i, numfds = tgt_len / sizeof(int);
 
                 for (i = 0; i < numfds; i++)
                     target_fd[i] = tswap32(fd[i]);
@@ -1302,8 +1343,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 struct target_timeval *target_tv =
                     (struct target_timeval *)target_data;
 
-                if (len != sizeof(struct timeval))
+                if (len != sizeof(struct timeval) ||
+                    tgt_len != sizeof(struct target_timeval)) {
                     goto unimplemented;
+                }
 
                 /* copy struct timeval to target */
                 target_tv->tv_sec = tswapal(tv->tv_sec);
@@ -1330,9 +1373,19 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         unimplemented:
             gemu_log("Unsupported ancillary data: %d/%d\n",
                                         cmsg->cmsg_level, cmsg->cmsg_type);
-            memcpy(target_data, data, len);
+            memcpy(target_data, data, MIN(len, tgt_len));
+            if (tgt_len > len) {
+                memset(target_data + len, 0, tgt_len - len);
+            }
         }
 
+        target_cmsg->cmsg_len = tswapal(tgt_len);
+        tgt_space = TARGET_CMSG_SPACE(tgt_len);
+        if (msg_controllen < tgt_space) {
+            tgt_space = msg_controllen;
+        }
+        msg_controllen -= tgt_space;
+        space += tgt_space;
         cmsg = CMSG_NXTHDR(msgh, cmsg);
         target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
     }
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] linux-user: use __get_user and __put_user in cmsg conversions

[Peter Maydell]

The target payloads in cmsg conversions may not have the alignment
required by the host. Using the get_user and put_user functions is
the easiest way to handle this and also do the byte-swapping we
require.

(Note that prior to this commit target_to_host_cmsg was incorrectly
using __put_user() rather than __get_user() for the SCM_CREDENTIALS
conversion, which meant it wasn't getting the benefit of the
misalignment handling.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3db6a90..7ef31b7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1228,17 +1228,18 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
             int *target_fd = (int *)target_data;
             int i, numfds = len / sizeof(int);
 
-            for (i = 0; i < numfds; i++)
-                fd[i] = tswap32(target_fd[i]);
+            for (i = 0; i < numfds; i++) {
+                __get_user(fd[i], target_fd + i);
+            }
         } else if (cmsg->cmsg_level == SOL_SOCKET
                &&  cmsg->cmsg_type == SCM_CREDENTIALS) {
             struct ucred *cred = (struct ucred *)data;
             struct target_ucred *target_cred =
                 (struct target_ucred *)target_data;
 
-            __put_user(target_cred->pid, &cred->pid);
-            __put_user(target_cred->uid, &cred->uid);
-            __put_user(target_cred->gid, &cred->gid);
+            __get_user(cred->pid, &target_cred->pid);
+            __get_user(cred->uid, &target_cred->uid);
+            __get_user(cred->gid, &target_cred->gid);
         } else {
             gemu_log("Unsupported ancillary data: %d/%d\n",
                                         cmsg->cmsg_level, cmsg->cmsg_type);
@@ -1333,8 +1334,9 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 int *target_fd = (int *)target_data;
                 int i, numfds = tgt_len / sizeof(int);
 
-                for (i = 0; i < numfds; i++)
-                    target_fd[i] = tswap32(fd[i]);
+                for (i = 0; i < numfds; i++) {
+                    __put_user(fd[i], target_fd + i);
+                }
                 break;
             }
             case SO_TIMESTAMP:
@@ -1349,8 +1351,8 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 }
 
                 /* copy struct timeval to target */
-                target_tv->tv_sec = tswapal(tv->tv_sec);
-                target_tv->tv_usec = tswapal(tv->tv_usec);
+                __put_user(tv->tv_sec, &target_tv->tv_sec);
+                __put_user(tv->tv_usec, &target_tv->tv_usec);
                 break;
             }
             case SCM_CREDENTIALS:
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix length handling in cmsg conversions

[Peter Maydell]

Ping?

thanks
-- PMM

On 26 May 2015 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset fixes some problems in conversions of cmsg structures
> in target_to_host_cmsg() (used in send/recvmsg handling). Specifically:
>
>      * we required the msg->msg_controllen to declare the buffer
>        to have enough space for final trailing padding (we were
>        checking against CMSG_SPACE), whereas the kernel does not
>        require this, and common userspace code assumes this.
>      * we weren't correctly handling the fact that the SO_TIMESTAMP
>        payload may be larger for the target than the host
>      * we weren't marking the messages with MSG_CTRUNC when we did
>        need to truncate a message that wasn't truncated by the host,
>        but were instead logging a QEMU message; since truncation is
>        always the result of a guest giving us an insufficiently
>        sized buffer, we should report it to the guest as the kernel
>        does and don't log anything
>      * we weren't handling the possibility of the host having a
>        more restrictive alignment requirement for payload structs
>
> The major visible issue I wanted to fix is that glibc's "try to talk
> to nscd" code that it will run on startup will receive a cmsg with a
> 4 byte payload and only allocates 4 bytes for it, which was causing
> us to do the wrong thing on architectures that need 8-alignment
> (we dropped the cmsg and printed a diagnostic message).
>
>
> Peter Maydell (2):
>   linux-user: Fix length handling in host_to_target_cmsg
>   linux-user: use __get_user and __put_user in cmsg conversions
>
>  linux-user/syscall.c | 89 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 72 insertions(+), 17 deletions(-)
>
> --
> 1.9.1

Re: [Qemu-devel] [PATCH 0/2] linux-user: Fix length handling in cmsg conversions

[Riku Voipio]

On Jun 5, 2015 6:03 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> Ping?

I'll collect and review linux-user patches from the list on monday.

Riku

>
> thanks 
> -- PMM 
>
> On 26 May 2015 at 19:46, Peter Maydell <peter.maydell@linaro.org> wrote: 
> > This patchset fixes some problems in conversions of cmsg structures 
> > in target_to_host_cmsg() (used in send/recvmsg handling). Specifically: 
> > 
> >      * we required the msg->msg_controllen to declare the buffer 
> >        to have enough space for final trailing padding (we were 
> >        checking against CMSG_SPACE), whereas the kernel does not 
> >        require this, and common userspace code assumes this. 
> >      * we weren't correctly handling the fact that the SO_TIMESTAMP 
> >        payload may be larger for the target than the host 
> >      * we weren't marking the messages with MSG_CTRUNC when we did 
> >        need to truncate a message that wasn't truncated by the host, 
> >        but were instead logging a QEMU message; since truncation is 
> >        always the result of a guest giving us an insufficiently 
> >        sized buffer, we should report it to the guest as the kernel 
> >        does and don't log anything 
> >      * we weren't handling the possibility of the host having a 
> >        more restrictive alignment requirement for payload structs 
> > 
> > The major visible issue I wanted to fix is that glibc's "try to talk 
> > to nscd" code that it will run on startup will receive a cmsg with a 
> > 4 byte payload and only allocates 4 bytes for it, which was causing 
> > us to do the wrong thing on architectures that need 8-alignment 
> > (we dropped the cmsg and printed a diagnostic message). 
> > 
> > 
> > Peter Maydell (2): 
> >   linux-user: Fix length handling in host_to_target_cmsg 
> >   linux-user: use __get_user and __put_user in cmsg conversions 
> > 
> >  linux-user/syscall.c | 89 ++++++++++++++++++++++++++++++++++++++++++---------- 
> >  1 file changed, 72 insertions(+), 17 deletions(-) 
> > 
> > -- 
> > 1.9.1