* [Qemu-devel] [PATCH 0/2] linux-user: Fix length calculations in host_to_target_cmsg() @ 2017-12-15 13:52 Peter Maydell 2017-12-15 13:52 ` [Qemu-devel] [PATCH 1/2] " Peter Maydell 2017-12-15 13:52 ` [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) Peter Maydell 0 siblings, 2 replies; 6+ messages in thread From: Peter Maydell @ 2017-12-15 13:52 UTC (permalink / raw) To: qemu-devel; +Cc: patches, Riku Voipio, Laurent Vivier, Bruno Haible 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 1/2] linux-user: Fix length calculations in host_to_target_cmsg() 2017-12-15 13:52 [Qemu-devel] [PATCH 0/2] linux-user: Fix length calculations in host_to_target_cmsg() Peter Maydell @ 2017-12-15 13:52 ` Peter Maydell 2018-01-19 16:32 ` Laurent Vivier 2017-12-15 13:52 ` [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) Peter Maydell 1 sibling, 1 reply; 6+ messages in thread From: Peter Maydell @ 2017-12-15 13:52 UTC (permalink / raw) To: qemu-devel; +Cc: patches, Riku Voipio, Laurent Vivier, Bruno Haible 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] linux-user: Fix length calculations in host_to_target_cmsg() 2017-12-15 13:52 ` [Qemu-devel] [PATCH 1/2] " Peter Maydell @ 2018-01-19 16:32 ` Laurent Vivier 0 siblings, 0 replies; 6+ messages in thread From: Laurent Vivier @ 2018-01-19 16:32 UTC (permalink / raw) To: Peter Maydell, qemu-devel; +Cc: Riku Voipio, Bruno Haible, patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) 2017-12-15 13:52 [Qemu-devel] [PATCH 0/2] linux-user: Fix length calculations in host_to_target_cmsg() Peter Maydell 2017-12-15 13:52 ` [Qemu-devel] [PATCH 1/2] " Peter Maydell @ 2017-12-15 13:52 ` Peter Maydell 2017-12-15 14:29 ` Laurent Vivier 2018-01-19 16:33 ` Laurent Vivier 1 sibling, 2 replies; 6+ messages in thread From: Peter Maydell @ 2017-12-15 13:52 UTC (permalink / raw) To: qemu-devel; +Cc: patches, Riku Voipio, Laurent Vivier, Bruno Haible 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) 2017-12-15 13:52 ` [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) Peter Maydell @ 2017-12-15 14:29 ` Laurent Vivier 2018-01-19 16:33 ` Laurent Vivier 1 sibling, 0 replies; 6+ messages in thread From: Laurent Vivier @ 2017-12-15 14:29 UTC (permalink / raw) To: Peter Maydell, qemu-devel; +Cc: Riku Voipio, Bruno Haible, patches 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) 2017-12-15 13:52 ` [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) Peter Maydell 2017-12-15 14:29 ` Laurent Vivier @ 2018-01-19 16:33 ` Laurent Vivier 1 sibling, 0 replies; 6+ messages in thread From: Laurent Vivier @ 2018-01-19 16:33 UTC (permalink / raw) To: Peter Maydell, qemu-devel; +Cc: Riku Voipio, Bruno Haible, patches 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-01-19 16:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-15 13:52 [Qemu-devel] [PATCH 0/2] linux-user: Fix length calculations in host_to_target_cmsg() Peter Maydell 2017-12-15 13:52 ` [Qemu-devel] [PATCH 1/2] " Peter Maydell 2018-01-19 16:32 ` Laurent Vivier 2017-12-15 13:52 ` [Qemu-devel] [PATCH 2/2] linux-user: Don't use CMSG_ALIGN(sizeof struct cmsghdr) Peter Maydell 2017-12-15 14:29 ` Laurent Vivier 2018-01-19 16:33 ` Laurent Vivier
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.