[Qemu-devel] [PATCH 0/2] linux-user: Fix length calculations in host_to_target_cmsg()

[Qemu-devel] [PATCH 0/2] linux-user: Fix length calculations in host_to_target_cmsg()

[Peter Maydell]

This patchset fixes this bug:
https://bugs.launchpad.net/qemu/+bug/1701808

where we were getting our message length calculations in
host_to_target_cmsg() wrong and could thus overwrite the end of the
guest buffer when we tried to fill it with the results of a
recvmsg().

Patch 1 is the bugfix; patch 2 is a minor cleanup which removes some
unnecessary and confusing alignment arithmetic and brings us into
alignment with what the kernel currently does.

thanks
-- PMM

Peter Maydell (2):
  linux-user: Fix length calculations in host_to_target_cmsg()
  linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr)

 linux-user/syscall_defs.h |  6 +++---
 linux-user/syscall.c      | 33 ++++++++++++++++++++++++---------
 2 files changed, 27 insertions(+), 12 deletions(-)

-- 
2.7.4

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

[Peter Maydell]

The handling of length calculations in host_to_target_cmsg()
was rather confused:
 * when checking for whether the target cmsg header fit in
   the remaining buffer, we were using the host struct size,
   not the target size
 * we were setting tgt_len to "target payload + header length"
   but then using it as if it were the target payload length alone
 * in various message type cases we weren't handling the possibility
   that host or target buffers were truncated

Fix these problems. The second one in particular is liable
to result in us overrunning the guest provided buffer,
since we will try to convert more data than is actually
present.

Fixes: https://bugs.launchpad.net/qemu/+bug/1701808
Reported-by: Bruno Haible  <bruno@clisp.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 11c9116..a1b9772 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1782,7 +1782,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
          * 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)) {
+        if (msg_controllen < sizeof(struct target_cmsghdr)) {
             target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
             break;
         }
@@ -1794,8 +1794,6 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         }
         target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
 
-        tgt_len = TARGET_CMSG_LEN(len);
-
         /* Payload types which need a different size of payload on
          * the target must adjust tgt_len here.
          */
@@ -1809,12 +1807,13 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 break;
             }
         default:
+            tgt_len = len;
             break;
         }
 
-        if (msg_controllen < tgt_len) {
+        if (msg_controllen < TARGET_CMSG_LEN(tgt_len)) {
             target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
-            tgt_len = msg_controllen;
+            tgt_len = msg_controllen - sizeof(struct target_cmsghdr);
         }
 
         /* We must now copy-and-convert len bytes of payload
@@ -1875,6 +1874,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 uint32_t *v = (uint32_t *)data;
                 uint32_t *t_int = (uint32_t *)target_data;
 
+                if (len != sizeof(uint32_t) ||
+                    tgt_len != sizeof(uint32_t)) {
+                    goto unimplemented;
+                }
                 __put_user(*v, t_int);
                 break;
             }
@@ -1888,6 +1891,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 struct errhdr_t *target_errh =
                     (struct errhdr_t *)target_data;
 
+                if (len != sizeof(struct errhdr_t) ||
+                    tgt_len != sizeof(struct errhdr_t)) {
+                    goto unimplemented;
+                }
                 __put_user(errh->ee.ee_errno, &target_errh->ee.ee_errno);
                 __put_user(errh->ee.ee_origin, &target_errh->ee.ee_origin);
                 __put_user(errh->ee.ee_type,  &target_errh->ee.ee_type);
@@ -1911,6 +1918,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 uint32_t *v = (uint32_t *)data;
                 uint32_t *t_int = (uint32_t *)target_data;
 
+                if (len != sizeof(uint32_t) ||
+                    tgt_len != sizeof(uint32_t)) {
+                    goto unimplemented;
+                }
                 __put_user(*v, t_int);
                 break;
             }
@@ -1924,6 +1935,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
                 struct errhdr6_t *target_errh =
                     (struct errhdr6_t *)target_data;
 
+                if (len != sizeof(struct errhdr6_t) ||
+                    tgt_len != sizeof(struct errhdr6_t)) {
+                    goto unimplemented;
+                }
                 __put_user(errh->ee.ee_errno, &target_errh->ee.ee_errno);
                 __put_user(errh->ee.ee_origin, &target_errh->ee.ee_origin);
                 __put_user(errh->ee.ee_type,  &target_errh->ee.ee_type);
@@ -1950,8 +1965,8 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
             }
         }
 
-        target_cmsg->cmsg_len = tswapal(tgt_len);
-        tgt_space = TARGET_CMSG_SPACE(len);
+        target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(tgt_len));
+        tgt_space = TARGET_CMSG_SPACE(tgt_len);
         if (msg_controllen < tgt_space) {
             tgt_space = msg_controllen;
         }
-- 
2.7.4

[Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr)

[Peter Maydell]

The Linux struct cmsghdr is already guaranteed to be sufficiently
aligned that CMSG_ALIGN(sizeof struct cmsghdr) is always equal
to sizeof struct cmsghdr. Stop doing the unnecessary alignment
arithmetic for host and target cmsghdr.

This follows kernel commit 1ff8cebf49ed9e9ca2 and brings our
TARGET_CMSG_* macros back into line with the kernel ones,
as well as making them easier to understand.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/syscall_defs.h | 6 +++---
 linux-user/syscall.c      | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index bec3680..a35c52a 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -303,9 +303,9 @@ struct target_cmsghdr {
                                __target_cmsg_nxthdr(mhdr, cmsg, cmsg_start)
 #define TARGET_CMSG_ALIGN(len) (((len) + sizeof (abi_long) - 1) \
                                & (size_t) ~(sizeof (abi_long) - 1))
-#define TARGET_CMSG_SPACE(len) (TARGET_CMSG_ALIGN (len) \
-                               + TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)))
-#define TARGET_CMSG_LEN(len)   (TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)) + (len))
+#define TARGET_CMSG_SPACE(len) (sizeof(struct target_cmsghdr) + \
+                                TARGET_CMSG_ALIGN(len))
+#define TARGET_CMSG_LEN(len) (sizeof(struct target_cmsghdr) + (len))
 
 static __inline__ struct target_cmsghdr *
 __target_cmsg_nxthdr(struct target_msghdr *__mhdr,
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a1b9772..39553c8 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1692,7 +1692,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         void *target_data = TARGET_CMSG_DATA(target_cmsg);
 
         int len = tswapal(target_cmsg->cmsg_len)
-                  - TARGET_CMSG_ALIGN(sizeof (struct target_cmsghdr));
+            - sizeof(struct target_cmsghdr);
 
         space += CMSG_SPACE(len);
         if (space > msgh->msg_controllen) {
@@ -1773,7 +1773,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         void *data = CMSG_DATA(cmsg);
         void *target_data = TARGET_CMSG_DATA(target_cmsg);
 
-        int len = cmsg->cmsg_len - CMSG_ALIGN(sizeof (struct cmsghdr));
+        int len = cmsg->cmsg_len - sizeof(struct cmsghdr);
         int tgt_len, tgt_space;
 
         /* We never copy a half-header but may copy half-data;
-- 
2.7.4

Re: [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr)

[Laurent Vivier]

Le 15/12/2017 à 14:52, Peter Maydell a écrit :
> The Linux struct cmsghdr is already guaranteed to be sufficiently
> aligned that CMSG_ALIGN(sizeof struct cmsghdr) is always equal
> to sizeof struct cmsghdr. Stop doing the unnecessary alignment
> arithmetic for host and target cmsghdr.
> 
> This follows kernel commit 1ff8cebf49ed9e9ca2 and brings our
> TARGET_CMSG_* macros back into line with the kernel ones,
> as well as making them easier to understand.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall_defs.h | 6 +++---
>  linux-user/syscall.c      | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH 1/2] linux-user: Fix length calculations in host_to_target_cmsg()

[Laurent Vivier]

Le 15/12/2017 à 14:52, Peter Maydell a écrit :
> The handling of length calculations in host_to_target_cmsg()
> was rather confused:
>  * when checking for whether the target cmsg header fit in
>    the remaining buffer, we were using the host struct size,
>    not the target size
>  * we were setting tgt_len to "target payload + header length"
>    but then using it as if it were the target payload length alone
>  * in various message type cases we weren't handling the possibility
>    that host or target buffers were truncated
> 
> Fix these problems. The second one in particular is liable
> to result in us overrunning the guest provided buffer,
> since we will try to convert more data than is actually
> present.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1701808
> Reported-by: Bruno Haible  <bruno@clisp.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 11c9116..a1b9772 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1782,7 +1782,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>           * 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)) {
> +        if (msg_controllen < sizeof(struct target_cmsghdr)) {
>              target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
>              break;
>          }
> @@ -1794,8 +1794,6 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>          }
>          target_cmsg->cmsg_type = tswap32(cmsg->cmsg_type);
>  
> -        tgt_len = TARGET_CMSG_LEN(len);
> -
>          /* Payload types which need a different size of payload on
>           * the target must adjust tgt_len here.
>           */
> @@ -1809,12 +1807,13 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>                  break;
>              }
>          default:
> +            tgt_len = len;
>              break;
>          }
>  
> -        if (msg_controllen < tgt_len) {
> +        if (msg_controllen < TARGET_CMSG_LEN(tgt_len)) {
>              target_msgh->msg_flags |= tswap32(MSG_CTRUNC);
> -            tgt_len = msg_controllen;
> +            tgt_len = msg_controllen - sizeof(struct target_cmsghdr);
>          }
>  
>          /* We must now copy-and-convert len bytes of payload
> @@ -1875,6 +1874,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>                  uint32_t *v = (uint32_t *)data;
>                  uint32_t *t_int = (uint32_t *)target_data;
>  
> +                if (len != sizeof(uint32_t) ||
> +                    tgt_len != sizeof(uint32_t)) {
> +                    goto unimplemented;
> +                }
>                  __put_user(*v, t_int);
>                  break;
>              }
> @@ -1888,6 +1891,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>                  struct errhdr_t *target_errh =
>                      (struct errhdr_t *)target_data;
>  
> +                if (len != sizeof(struct errhdr_t) ||
> +                    tgt_len != sizeof(struct errhdr_t)) {
> +                    goto unimplemented;
> +                }
>                  __put_user(errh->ee.ee_errno, &target_errh->ee.ee_errno);
>                  __put_user(errh->ee.ee_origin, &target_errh->ee.ee_origin);
>                  __put_user(errh->ee.ee_type,  &target_errh->ee.ee_type);
> @@ -1911,6 +1918,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>                  uint32_t *v = (uint32_t *)data;
>                  uint32_t *t_int = (uint32_t *)target_data;
>  
> +                if (len != sizeof(uint32_t) ||
> +                    tgt_len != sizeof(uint32_t)) {
> +                    goto unimplemented;
> +                }
>                  __put_user(*v, t_int);
>                  break;
>              }
> @@ -1924,6 +1935,10 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>                  struct errhdr6_t *target_errh =
>                      (struct errhdr6_t *)target_data;
>  
> +                if (len != sizeof(struct errhdr6_t) ||
> +                    tgt_len != sizeof(struct errhdr6_t)) {
> +                    goto unimplemented;
> +                }
>                  __put_user(errh->ee.ee_errno, &target_errh->ee.ee_errno);
>                  __put_user(errh->ee.ee_origin, &target_errh->ee.ee_origin);
>                  __put_user(errh->ee.ee_type,  &target_errh->ee.ee_type);
> @@ -1950,8 +1965,8 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>              }
>          }
>  
> -        target_cmsg->cmsg_len = tswapal(tgt_len);
> -        tgt_space = TARGET_CMSG_SPACE(len);
> +        target_cmsg->cmsg_len = tswapal(TARGET_CMSG_LEN(tgt_len));
> +        tgt_space = TARGET_CMSG_SPACE(tgt_len);
>          if (msg_controllen < tgt_space) {
>              tgt_space = msg_controllen;
>          }
> 

Applied to my linux-user branch.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr)

[Laurent Vivier]

Le 15/12/2017 à 14:52, Peter Maydell a écrit :
> The Linux struct cmsghdr is already guaranteed to be sufficiently
> aligned that CMSG_ALIGN(sizeof struct cmsghdr) is always equal
> to sizeof struct cmsghdr. Stop doing the unnecessary alignment
> arithmetic for host and target cmsghdr.
> 
> This follows kernel commit 1ff8cebf49ed9e9ca2 and brings our
> TARGET_CMSG_* macros back into line with the kernel ones,
> as well as making them easier to understand.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  linux-user/syscall_defs.h | 6 +++---
>  linux-user/syscall.c      | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index bec3680..a35c52a 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -303,9 +303,9 @@ struct target_cmsghdr {
>                                 __target_cmsg_nxthdr(mhdr, cmsg, cmsg_start)
>  #define TARGET_CMSG_ALIGN(len) (((len) + sizeof (abi_long) - 1) \
>                                 & (size_t) ~(sizeof (abi_long) - 1))
> -#define TARGET_CMSG_SPACE(len) (TARGET_CMSG_ALIGN (len) \
> -                               + TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)))
> -#define TARGET_CMSG_LEN(len)   (TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)) + (len))
> +#define TARGET_CMSG_SPACE(len) (sizeof(struct target_cmsghdr) + \
> +                                TARGET_CMSG_ALIGN(len))
> +#define TARGET_CMSG_LEN(len) (sizeof(struct target_cmsghdr) + (len))
>  
>  static __inline__ struct target_cmsghdr *
>  __target_cmsg_nxthdr(struct target_msghdr *__mhdr,
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a1b9772..39553c8 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1692,7 +1692,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
>          void *target_data = TARGET_CMSG_DATA(target_cmsg);
>  
>          int len = tswapal(target_cmsg->cmsg_len)
> -                  - TARGET_CMSG_ALIGN(sizeof (struct target_cmsghdr));
> +            - sizeof(struct target_cmsghdr);
>  
>          space += CMSG_SPACE(len);
>          if (space > msgh->msg_controllen) {
> @@ -1773,7 +1773,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>          void *data = CMSG_DATA(cmsg);
>          void *target_data = TARGET_CMSG_DATA(target_cmsg);
>  
> -        int len = cmsg->cmsg_len - CMSG_ALIGN(sizeof (struct cmsghdr));
> +        int len = cmsg->cmsg_len - sizeof(struct cmsghdr);
>          int tgt_len, tgt_space;
>  
>          /* We never copy a half-header but may copy half-data;
> 

Applied to my linux-user branch.

Thanks,
Laurent