| Message ID | 20150903052726.GD1761@latitude |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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 >
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
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 {
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(-)