[Qemu-devel] [PATCH] linux-user: fix host_to_target_cmsg in case of multiple headers

[Qemu-devel] [PATCH] linux-user: fix host_to_target_cmsg in case of multiple headers

[Jonathan Neuschäfer]

In the current implementation, __target_cmsg_nxthdr compares a pointer
derived from target_cmsg against the msg_control field of target_msgh
(through subtraction).  This failed for me when emulating i386 code
under x86_64, because pointers in the host address space and pointers in
the guest address space were not the same.  This patch adds a g2h()
address translation around the msg_control value.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 linux-user/syscall_defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index edd5f3c..1eaaf2a 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -248,7 +248,7 @@ __target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cms
 
   __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
                                     + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
-  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
+  if ((unsigned long)((char *)(__ptr+1) - (char *)g2h(tswapal(__mhdr->msg_control)))
       > tswapal(__mhdr->msg_controllen))
     /* No more entries.  */
     return (struct target_cmsghdr *)0;
-- 
2.5.0

Re: [Qemu-devel] [PATCH] linux-user: fix host_to_target_cmsg in case of multiple headers

[Peter Maydell]

On 27 August 2015 at 15:50, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> In the current implementation, __target_cmsg_nxthdr compares a pointer
> derived from target_cmsg against the msg_control field of target_msgh
> (through subtraction).  This failed for me when emulating i386 code
> under x86_64, because pointers in the host address space and pointers in
> the guest address space were not the same.  This patch adds a g2h()
> address translation around the msg_control value.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  linux-user/syscall_defs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index edd5f3c..1eaaf2a 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -248,7 +248,7 @@ __target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cms
>
>    __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
>                                      + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
> -  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
> +  if ((unsigned long)((char *)(__ptr+1) - (char *)g2h(tswapal(__mhdr->msg_control)))
>        > tswapal(__mhdr->msg_controllen))
>      /* No more entries.  */
>      return (struct target_cmsghdr *)0;

This definitely looks like a bug, but I don't think this is
a sufficient fix, because if DEBUG_REMAP is defined then the
locked-memory which the target_cmsghdr* is in is not a
simple g2h() away from the host pointer.

What you need to do is change target_to_host_cmsg and
host_to_target_cmsg so that when at the top of the function
we do:
    target_cmsg_addr = tswapal(target_msgh->msg_control);
    target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 1);

we save that target_cmsg into some variable (eg target_msg_control),
and pass it into the TARGET_CMSG_NXTHDR macro. That host pointer
is the one we need to use on the right side of the subtraction.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: fix host_to_target_cmsg in case of multiple headers

[Jonathan Neuschäfer]

On Thu, Aug 27, 2015 at 07:06:24PM +0100, Peter Maydell wrote:
> This definitely looks like a bug, but I don't think this is
> a sufficient fix, because if DEBUG_REMAP is defined then the
> locked-memory which the target_cmsghdr* is in is not a
> simple g2h() away from the host pointer.
> 
> What you need to do is change target_to_host_cmsg and
> host_to_target_cmsg so that when at the top of the function
> we do:
>     target_cmsg_addr = tswapal(target_msgh->msg_control);
>     target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 1);

After following your advice, I found two more bugs in related code,
which I had to fix. I probably missed something, but my test case
(wine), which receives two headers via a single recvmsg call, is now
working properly. Perhaps an isolated test case would be nice.

I'll send my new patch soon.


Regards,
Jonathan Neuschäfer

Re: [Qemu-devel] [PATCH] linux-user: fix host_to_target_cmsg in case of multiple headers

[Peter Maydell]

On 29 August 2015 at 17:04, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> On Thu, Aug 27, 2015 at 07:06:24PM +0100, Peter Maydell wrote:
>> This definitely looks like a bug, but I don't think this is
>> a sufficient fix, because if DEBUG_REMAP is defined then the
>> locked-memory which the target_cmsghdr* is in is not a
>> simple g2h() away from the host pointer.
>>
>> What you need to do is change target_to_host_cmsg and
>> host_to_target_cmsg so that when at the top of the function
>> we do:
>>     target_cmsg_addr = tswapal(target_msgh->msg_control);
>>     target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 1);
>
> After following your advice, I found two more bugs in related code,
> which I had to fix. I probably missed something, but my test case
> (wine), which receives two headers via a single recvmsg call, is now
> working properly. Perhaps an isolated test case would be nice.

I generally test linux-user with the LTP tests:
http://wiki.qemu.org/Testing/LTP
though they're not always as good at flushing out QEMU
bugs as you might hope. Still, LTP is probably the best
home for syscall tests.

thanks
-- PMM

[Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Jonathan Neuschäfer]

Currently, __target_cmsg_nxthdr compares a pointer derived from
target_cmsg against the msg_control field of target_msgh (through
subtraction).  This failed for me when emulating i386 code under x86_64,
because pointers in the host address space and pointers in the guest
address space were not the same.  This patch passes the initial value of
target_cmsg into __target_cmsg_nxthdr.

I found and fixed two more related bugs:
 - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
   old one.
 - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
   target_cmsghdr)" twice anymore.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

--
Changes since v1:
- Follow Peter Maydell's advice on how to fix the first bug
- The "two more related bugs"
---
 linux-user/syscall.c      | 14 +++++++++-----
 linux-user/syscall_defs.h |  9 +++++----
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f62c698..12a6cd2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1181,7 +1181,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
     struct cmsghdr *cmsg = CMSG_FIRSTHDR(msgh);
     abi_long msg_controllen;
     abi_ulong target_cmsg_addr;
-    struct target_cmsghdr *target_cmsg;
+    struct target_cmsghdr *target_cmsg, *target_cmsg_start;
     socklen_t space = 0;
     
     msg_controllen = tswapal(target_msgh->msg_controllen);
@@ -1189,6 +1189,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         goto the_end;
     target_cmsg_addr = tswapal(target_msgh->msg_control);
     target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 1);
+    target_cmsg_start = target_cmsg;
     if (!target_cmsg)
         return -TARGET_EFAULT;
 
@@ -1247,7 +1248,8 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         }
 
         cmsg = CMSG_NXTHDR(msgh, cmsg);
-        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
+        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg,
+                                         target_cmsg_start);
     }
     unlock_user(target_cmsg, target_cmsg_addr, 0);
  the_end:
@@ -1261,7 +1263,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
     struct cmsghdr *cmsg = CMSG_FIRSTHDR(msgh);
     abi_long msg_controllen;
     abi_ulong target_cmsg_addr;
-    struct target_cmsghdr *target_cmsg;
+    struct target_cmsghdr *target_cmsg, *target_cmsg_start;
     socklen_t space = 0;
 
     msg_controllen = tswapal(target_msgh->msg_controllen);
@@ -1269,6 +1271,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         goto the_end;
     target_cmsg_addr = tswapal(target_msgh->msg_control);
     target_cmsg = lock_user(VERIFY_WRITE, target_cmsg_addr, msg_controllen, 0);
+    target_cmsg_start = target_cmsg;
     if (!target_cmsg)
         return -TARGET_EFAULT;
 
@@ -1382,14 +1385,15 @@ 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(tgt_len);
+        tgt_space = TARGET_CMSG_SPACE(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);
+        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg,
+                                         target_cmsg_start);
     }
     unlock_user(target_cmsg, target_cmsg_addr, space);
  the_end:
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index edd5f3c..8b0e354 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -234,7 +234,8 @@ struct target_cmsghdr {
 };
 
 #define TARGET_CMSG_DATA(cmsg) ((unsigned char *) ((struct target_cmsghdr *) (cmsg) + 1))
-#define TARGET_CMSG_NXTHDR(mhdr, cmsg) __target_cmsg_nxthdr (mhdr, cmsg)
+#define TARGET_CMSG_NXTHDR(mhdr, cmsg, cmsg_start) \
+                               __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) \
@@ -242,17 +243,17 @@ struct target_cmsghdr {
 #define TARGET_CMSG_LEN(len)   (TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)) + (len))
 
 static __inline__ struct target_cmsghdr *
-__target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cmsg)
+__target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cmsg, struct target_cmsghdr *__cmsg_start)
 {
   struct target_cmsghdr *__ptr;
 
   __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
                                     + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
-  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
+  if ((unsigned long)((char *)(__ptr+1) - (char *)__cmsg_start)
       > tswapal(__mhdr->msg_controllen))
     /* No more entries.  */
     return (struct target_cmsghdr *)0;
-  return __cmsg;
+  return __ptr;
 }
 
 struct target_mmsghdr {
-- 
2.5.0

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Peter Maydell]

On 31 August 2015 at 20:27, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> Currently, __target_cmsg_nxthdr compares a pointer derived from
> target_cmsg against the msg_control field of target_msgh (through
> subtraction).  This failed for me when emulating i386 code under x86_64,
> because pointers in the host address space and pointers in the guest
> address space were not the same.  This patch passes the initial value of
> target_cmsg into __target_cmsg_nxthdr.
>
> I found and fixed two more related bugs:
>  - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
>    old one.
>  - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
>    target_cmsghdr)" twice anymore.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

Code-wise this looks good; thanks. There are a few minor style
nits caught by scripts/checkpatch.pl, which it would be nice if
you can fix. (They're mostly things that were present in the code
already, but we like to fix the style issues when a patch has
to touch a line of code anyway.)

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Jonathan Neuschäfer]

On Tue, Sep 01, 2015 at 04:45:12PM +0100, Peter Maydell wrote:
> Code-wise this looks good; thanks. There are a few minor style
> nits caught by scripts/checkpatch.pl, which it would be nice if
> you can fix. (They're mostly things that were present in the code
> already, but we like to fix the style issues when a patch has
> to touch a line of code anyway.)

Okay, I'll fix the style issues.


--
Jonathan

[Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Jonathan Neuschäfer]

Currently, __target_cmsg_nxthdr compares a pointer derived from
target_cmsg against the msg_control field of target_msgh (through
subtraction).  This failed for me when emulating i386 code under x86_64,
because pointers in the host address space and pointers in the guest
address space were not the same.  This patch passes the initial value of
target_cmsg into __target_cmsg_nxthdr.

I found and fixed two more related bugs:
- __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
  old one.
- tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
  target_cmsghdr)" twice anymore.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

--
Changes since v2:
- The patch is now clean WRT checkpatch.pl
Changes since v1:
- Follow Peter Maydell's advice on how to fix the first bug
- The "two more related bugs"
---
 linux-user/syscall.c      | 14 +++++++++-----
 linux-user/syscall_defs.h | 14 +++++++++-----
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f62c698..12a6cd2 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1181,7 +1181,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
     struct cmsghdr *cmsg = CMSG_FIRSTHDR(msgh);
     abi_long msg_controllen;
     abi_ulong target_cmsg_addr;
-    struct target_cmsghdr *target_cmsg;
+    struct target_cmsghdr *target_cmsg, *target_cmsg_start;
     socklen_t space = 0;
     
     msg_controllen = tswapal(target_msgh->msg_controllen);
@@ -1189,6 +1189,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         goto the_end;
     target_cmsg_addr = tswapal(target_msgh->msg_control);
     target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 1);
+    target_cmsg_start = target_cmsg;
     if (!target_cmsg)
         return -TARGET_EFAULT;
 
@@ -1247,7 +1248,8 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
         }
 
         cmsg = CMSG_NXTHDR(msgh, cmsg);
-        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
+        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg,
+                                         target_cmsg_start);
     }
     unlock_user(target_cmsg, target_cmsg_addr, 0);
  the_end:
@@ -1261,7 +1263,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
     struct cmsghdr *cmsg = CMSG_FIRSTHDR(msgh);
     abi_long msg_controllen;
     abi_ulong target_cmsg_addr;
-    struct target_cmsghdr *target_cmsg;
+    struct target_cmsghdr *target_cmsg, *target_cmsg_start;
     socklen_t space = 0;
 
     msg_controllen = tswapal(target_msgh->msg_controllen);
@@ -1269,6 +1271,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
         goto the_end;
     target_cmsg_addr = tswapal(target_msgh->msg_control);
     target_cmsg = lock_user(VERIFY_WRITE, target_cmsg_addr, msg_controllen, 0);
+    target_cmsg_start = target_cmsg;
     if (!target_cmsg)
         return -TARGET_EFAULT;
 
@@ -1382,14 +1385,15 @@ 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(tgt_len);
+        tgt_space = TARGET_CMSG_SPACE(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);
+        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg,
+                                         target_cmsg_start);
     }
     unlock_user(target_cmsg, target_cmsg_addr, space);
  the_end:
diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
index edd5f3c..9d3c537 100644
--- a/linux-user/syscall_defs.h
+++ b/linux-user/syscall_defs.h
@@ -234,7 +234,8 @@ struct target_cmsghdr {
 };
 
 #define TARGET_CMSG_DATA(cmsg) ((unsigned char *) ((struct target_cmsghdr *) (cmsg) + 1))
-#define TARGET_CMSG_NXTHDR(mhdr, cmsg) __target_cmsg_nxthdr (mhdr, cmsg)
+#define TARGET_CMSG_NXTHDR(mhdr, cmsg, cmsg_start) \
+                               __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) \
@@ -242,17 +243,20 @@ struct target_cmsghdr {
 #define TARGET_CMSG_LEN(len)   (TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)) + (len))
 
 static __inline__ struct target_cmsghdr *
-__target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cmsg)
+__target_cmsg_nxthdr(struct target_msghdr *__mhdr,
+                     struct target_cmsghdr *__cmsg,
+                     struct target_cmsghdr *__cmsg_start)
 {
   struct target_cmsghdr *__ptr;
 
   __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
                                     + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
-  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
-      > tswapal(__mhdr->msg_controllen))
+  if ((unsigned long)((char *)(__ptr+1) - (char *)__cmsg_start)
+      > tswapal(__mhdr->msg_controllen)) {
     /* No more entries.  */
     return (struct target_cmsghdr *)0;
-  return __cmsg;
+  }
+  return __ptr;
 }
 
 struct target_mmsghdr {
-- 
2.5.0

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Peter Maydell]

On 3 September 2015 at 06:27, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> Currently, __target_cmsg_nxthdr compares a pointer derived from
> target_cmsg against the msg_control field of target_msgh (through
> subtraction).  This failed for me when emulating i386 code under x86_64,
> because pointers in the host address space and pointers in the guest
> address space were not the same.  This patch passes the initial value of
> target_cmsg into __target_cmsg_nxthdr.
>
> I found and fixed two more related bugs:
> - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
>   old one.
> - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
>   target_cmsghdr)" twice anymore.
>
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Jonathan Neuschäfer]

On Fri, Sep 04, 2015 at 01:48:39PM +0100, Peter Maydell wrote:
> On 3 September 2015 at 06:27, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> > Currently, __target_cmsg_nxthdr compares a pointer derived from
> > target_cmsg against the msg_control field of target_msgh (through
> > subtraction).  This failed for me when emulating i386 code under x86_64,
> > because pointers in the host address space and pointers in the guest
> > address space were not the same.  This patch passes the initial value of
> > target_cmsg into __target_cmsg_nxthdr.
> >
> > I found and fixed two more related bugs:
> > - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
> >   old one.
> > - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
> >   target_cmsghdr)" twice anymore.
> >
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Ping.

What's the status of this patch?


Regards,
Jonathan

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Peter Maydell]

On 20 September 2015 at 22:34, Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:
> On Fri, Sep 04, 2015 at 01:48:39PM +0100, Peter Maydell wrote:
>> On 3 September 2015 at 06:27, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
>> > Currently, __target_cmsg_nxthdr compares a pointer derived from
>> > target_cmsg against the msg_control field of target_msgh (through
>> > subtraction).  This failed for me when emulating i386 code under x86_64,
>> > because pointers in the host address space and pointers in the guest
>> > address space were not the same.  This patch passes the initial value of
>> > target_cmsg into __target_cmsg_nxthdr.
>> >
>> > I found and fixed two more related bugs:
>> > - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
>> >   old one.
>> > - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
>> >   target_cmsghdr)" twice anymore.
>> >
>> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
>>
>> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Ping.
>
> What's the status of this patch?

It's waiting for Riku to wake up and put it into a linux-user
pull request.

thanks
-- MPM

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Riku Voipio]

On Mon, Sep 21, 2015 at 06:13:57AM -0700, Peter Maydell wrote:
> On 20 September 2015 at 22:34, Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> wrote:
> > On Fri, Sep 04, 2015 at 01:48:39PM +0100, Peter Maydell wrote:
> >> On 3 September 2015 at 06:27, Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> >> > Currently, __target_cmsg_nxthdr compares a pointer derived from
> >> > target_cmsg against the msg_control field of target_msgh (through
> >> > subtraction).  This failed for me when emulating i386 code under x86_64,
> >> > because pointers in the host address space and pointers in the guest
> >> > address space were not the same.  This patch passes the initial value of
> >> > target_cmsg into __target_cmsg_nxthdr.
> >> >
> >> > I found and fixed two more related bugs:
> >> > - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
> >> >   old one.
> >> > - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
> >> >   target_cmsghdr)" twice anymore.
> >> >
> >> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> >>
> >> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> >
> > Ping.
> >
> > What's the status of this patch?
 
> It's waiting for Riku to wake up and put it into a linux-user
> pull request.

My intention was to gather a pull request before this travel, but
alas I was stuck with other stuff. I'll get it done next week latest.

Riku

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Jonathan Neuschäfer]

On Mon, Sep 21, 2015 at 12:45:21PM -0700, Riku Voipio wrote:
> On Mon, Sep 21, 2015 at 06:13:57AM -0700, Peter Maydell wrote:
> > On 20 September 2015 at 22:34, Jonathan Neuschäfer
[...]
> > > What's the status of this patch?
>  
> > It's waiting for Riku to wake up and put it into a linux-user
> > pull request.
> 
> My intention was to gather a pull request before this travel, but
> alas I was stuck with other stuff. I'll get it done next week latest.

No problem, take your time. I was just worried that it might get lost.


--
Jonathan

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Riku Voipio]

Hi,

On Thu, Sep 03, 2015 at 07:27:26AM +0200, Jonathan Neuschäfer wrote:
> Currently, __target_cmsg_nxthdr compares a pointer derived from
> target_cmsg against the msg_control field of target_msgh (through
> subtraction).  This failed for me when emulating i386 code under x86_64,
> because pointers in the host address space and pointers in the guest
> address space were not the same.  This patch passes the initial value of
> target_cmsg into __target_cmsg_nxthdr.
> 
> I found and fixed two more related bugs:
> - __target_cmsg_nxthdr now returns the new cmsg pointer instead of the
>   old one.
> - tgt_space (in host_to_target_cmsg) doesn't count "sizeof (struct
>   target_cmsghdr)" twice anymore.

Applied to linux-user tree, thanks.

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> --

For next time, use three dashes "---" to separate version-to-version
change descriptions from commit messages. This way git will
automatically leave them out when doing git am.

> Changes since v2:
> - The patch is now clean WRT checkpatch.pl
> Changes since v1:
> - Follow Peter Maydell's advice on how to fix the first bug
> - The "two more related bugs"
> ---
>  linux-user/syscall.c      | 14 +++++++++-----
>  linux-user/syscall_defs.h | 14 +++++++++-----
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f62c698..12a6cd2 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -1181,7 +1181,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
>      struct cmsghdr *cmsg = CMSG_FIRSTHDR(msgh);
>      abi_long msg_controllen;
>      abi_ulong target_cmsg_addr;
> -    struct target_cmsghdr *target_cmsg;
> +    struct target_cmsghdr *target_cmsg, *target_cmsg_start;
>      socklen_t space = 0;
>      
>      msg_controllen = tswapal(target_msgh->msg_controllen);
> @@ -1189,6 +1189,7 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
>          goto the_end;
>      target_cmsg_addr = tswapal(target_msgh->msg_control);
>      target_cmsg = lock_user(VERIFY_READ, target_cmsg_addr, msg_controllen, 1);
> +    target_cmsg_start = target_cmsg;
>      if (!target_cmsg)
>          return -TARGET_EFAULT;
>  
> @@ -1247,7 +1248,8 @@ static inline abi_long target_to_host_cmsg(struct msghdr *msgh,
>          }
>  
>          cmsg = CMSG_NXTHDR(msgh, cmsg);
> -        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg);
> +        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg,
> +                                         target_cmsg_start);
>      }
>      unlock_user(target_cmsg, target_cmsg_addr, 0);
>   the_end:
> @@ -1261,7 +1263,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>      struct cmsghdr *cmsg = CMSG_FIRSTHDR(msgh);
>      abi_long msg_controllen;
>      abi_ulong target_cmsg_addr;
> -    struct target_cmsghdr *target_cmsg;
> +    struct target_cmsghdr *target_cmsg, *target_cmsg_start;
>      socklen_t space = 0;
>  
>      msg_controllen = tswapal(target_msgh->msg_controllen);
> @@ -1269,6 +1271,7 @@ static inline abi_long host_to_target_cmsg(struct target_msghdr *target_msgh,
>          goto the_end;
>      target_cmsg_addr = tswapal(target_msgh->msg_control);
>      target_cmsg = lock_user(VERIFY_WRITE, target_cmsg_addr, msg_controllen, 0);
> +    target_cmsg_start = target_cmsg;
>      if (!target_cmsg)
>          return -TARGET_EFAULT;
>  
> @@ -1382,14 +1385,15 @@ 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(tgt_len);
> +        tgt_space = TARGET_CMSG_SPACE(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);
> +        target_cmsg = TARGET_CMSG_NXTHDR(target_msgh, target_cmsg,
> +                                         target_cmsg_start);
>      }
>      unlock_user(target_cmsg, target_cmsg_addr, space);
>   the_end:
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index edd5f3c..9d3c537 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -234,7 +234,8 @@ struct target_cmsghdr {
>  };
>  
>  #define TARGET_CMSG_DATA(cmsg) ((unsigned char *) ((struct target_cmsghdr *) (cmsg) + 1))
> -#define TARGET_CMSG_NXTHDR(mhdr, cmsg) __target_cmsg_nxthdr (mhdr, cmsg)
> +#define TARGET_CMSG_NXTHDR(mhdr, cmsg, cmsg_start) \
> +                               __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) \
> @@ -242,17 +243,20 @@ struct target_cmsghdr {
>  #define TARGET_CMSG_LEN(len)   (TARGET_CMSG_ALIGN (sizeof (struct target_cmsghdr)) + (len))
>  
>  static __inline__ struct target_cmsghdr *
> -__target_cmsg_nxthdr (struct target_msghdr *__mhdr, struct target_cmsghdr *__cmsg)
> +__target_cmsg_nxthdr(struct target_msghdr *__mhdr,
> +                     struct target_cmsghdr *__cmsg,
> +                     struct target_cmsghdr *__cmsg_start)
>  {
>    struct target_cmsghdr *__ptr;
>  
>    __ptr = (struct target_cmsghdr *)((unsigned char *) __cmsg
>                                      + TARGET_CMSG_ALIGN (tswapal(__cmsg->cmsg_len)));
> -  if ((unsigned long)((char *)(__ptr+1) - (char *)(size_t)tswapal(__mhdr->msg_control))
> -      > tswapal(__mhdr->msg_controllen))
> +  if ((unsigned long)((char *)(__ptr+1) - (char *)__cmsg_start)
> +      > tswapal(__mhdr->msg_controllen)) {
>      /* No more entries.  */
>      return (struct target_cmsghdr *)0;
> -  return __cmsg;
> +  }
> +  return __ptr;
>  }
>  
>  struct target_mmsghdr {
> -- 
> 2.5.0
> 

Re: [Qemu-devel] [PATCH] linux-user: fix cmsg conversion in case of multiple headers

[Jonathan Neuschäfer]

On Mon, Sep 28, 2015 at 04:38:03PM +0300, Riku Voipio wrote:
> Applied to linux-user tree, thanks.

Thanks.

> For next time, use three dashes "---" to separate version-to-version
> change descriptions from commit messages. This way git will
> automatically leave them out when doing git am.

Ok, I'll do that.


Jonathan