linux-user: fix host_to_target_cmsg in case of multiple headers

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 <[email protected]>

---
 linux-user/syscall_defs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

linux-user/syscall_defs.h +0 -0

index edd5f3c..1eaaf2a 100644

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);

Quoted 10 lines · Peter Maydell

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.

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 <[email protected]>

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.)

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 <[email protected]>

Quoted 13 lines · 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.

Quoted 1 line

Signed-off-by Jonathan Neuschäfer <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

Quoted 14 lines · Peter Maydell

Quoted 13 lines · 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.

Quoted 3 lines

Quoted 1 line

Signed-off-by Jonathan Neuschäfer <[email protected]>

Reviewed-by Peter Maydell <[email protected]>

Ping.

What's the status of this patch?

Quoted 17 lines · Jonathan Neuschäfer

Quoted 16 lines · Peter Maydell

Quoted 14 lines · 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 <[email protected]>

Quoted 5 lines

Quoted 1 line

Reviewed-by Peter Maydell <[email protected]>

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.

Quoted 1 line · Peter Maydell

On 20 September 2015 at 22:34, Jonathan Neuschäfer

Quoted 4 lines

Quoted 1 line

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.

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 <[email protected]>

--

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(-)

linux-user/syscall.c +9 -5

index f62c698..12a6cd2 100644
@@ -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:

linux-user/syscall_defs.h +9 -5

index edd5f3c..9d3c537 100644
@@ -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 {

Signature · 1 line

2.5.0

Applied to linux-user tree, 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.