| Message ID | 1464273450-31507-1-git-send-email-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 26 May 2016 11:37, Adhemerval Zanella wrote: > This patch fixes the remaining issue in sendmsg POSIX compliance by > adjusting the cmsghdr padding accordingly for 64-bits ABIs. Since > function contract does not allow to modify it in place, a temporary > buffer instead. Although the value used is arbitrary (current 2048 > bytes), it is expected to cover mostly common usar cases for this > facility (passing file descriptors and permission between processes). > > I did not send this change on previous patches because I would like > some feedback about buffer size used in copy operations. i think it should do a length test -- if it's below a threshold, use alloca, otherwise fall back to malloc+free. inserting our own limit here feels wrong. i guess scanning the reserved fields ahead of time to see if they are already zero isn't great either ? if people allocated the mem using calloc, then it's already zeroed ... > + struct cmsghdr auxcbuf[CMSGHDR_CONTROLLEN_MAX/sizeof(struct cmsghdr)+1]; style: missing spaces > + c = CMSG_NXTHDR(&hdr, c)) style: missing space before ( -mike
On Thu, 26 May 2016, Mike Frysinger wrote: > i think it should do a length test -- if it's below a threshold, use > alloca, otherwise fall back to malloc+free. inserting our own limit > here feels wrong. sendmsg is required by POSIX to be AS-safe, so can't use malloc+free; it would have to allocate memory in some other AS-safe way.
On 26/05/2016 13:27, Joseph Myers wrote: > On Thu, 26 May 2016, Mike Frysinger wrote: > >> i think it should do a length test -- if it's below a threshold, use >> alloca, otherwise fall back to malloc+free. inserting our own limit >> here feels wrong. > > sendmsg is required by POSIX to be AS-safe, so can't use malloc+free; it > would have to allocate memory in some other AS-safe way. And I think introducing AS-safe allocation here adds a lot of complexity and performance issues (even using mmap or pool buffer in some way). I do fell it might be wrong to add an arbitrary limit here, however the usage of control buffer in these syscalls is quite limited and passing large buffers is unusual. Also ENOMEM just instruct to use that it should either send multiple messages or rework de application, which I fell it is a tradeoff to implement the standard compliance fix.
On 26/05/2016 16:25, Zack Weinberg wrote: > > On May 26, 2016 3:37 PM, "Adhemerval Zanella" <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote: >> >> This patch fixes the remaining issue in sendmsg POSIX compliance by >> adjusting the cmsghdr padding accordingly for 64-bits ABIs. Since >> function contract does not allow to modify it in place, a temporary >> buffer instead. > > I don't think this can possibly be safe. It's not just a matter of the size limit; if there's *any way at all* for the caller to observe that a copy occurred -- that the pointer it supplied was not handed directly to the kernel -- including obscure cmsg types -- this will break some existing application. > I do not think this might be an issue since it is transparent to the application. The syscall contract handle both msghdr and cmsghdr as both constant data, so it does not matter afaik if msg_control points to a specific memory location, as long it fully represents the intended data meant to be passed along the syscall (which is what memcpy does). > Why do the __glibc_reserved1 fields have to be cleared in the first place? What reads from them? Why can't we fix that instead? It needs to be cleared because POSIX states the cmsg_len to be a socklen_t size, which for 64-bits architecture on Linux is still 32-bit unsigned types. So to be portable, programs can not rely on passing cmsghdr larger than socketlen_t size. > zw >
On 26/05/2016 18:07, Zack Weinberg wrote: > Please forgive terseness, I'm writing on a phone. > > On May 26, 2016 8:46 PM, "Adhemerval Zanella" <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote: >> On 26/05/2016 16:25, Zack Weinberg wrote: >> > On May 26, 2016 3:37 PM, "Adhemerval Zanella" <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote: >> >> >> >> This patch fixes the remaining issue in sendmsg POSIX compliance by >> >> adjusting the cmsghdr padding accordingly for 64-bits ABIs. Since >> >> function contract does not allow to modify it in place, a temporary >> >> buffer instead. >> > >> > I don't think this can possibly be safe. It's not just a matter of the size limit; if there's *any way at all* for the caller to observe that a copy occurred -- that the pointer it supplied was not handed directly to the kernel -- including obscure cmsg types -- this will break some existing application. >> > >> >> I do not think this might be an issue since it is transparent to the >> application. > > You haven't proven that to my satisfaction. It's not just about (c)msghdr pointing to read-only data > It's a question of whether there is now, or could conceivably be in the future, *any* way for caller to observe a copy. Since this interface is extensible by (at least) adding cmsg opcodes, that probably isn't a bar you can ever clear. It does not matter if the caller see or not a copy in sendmsg, what matter is if the syscall contract is respected. And afaik cmsghdr defined in a way where is ancillary is allocated using a flexibe array with its length defined by cmsg_len, so all the data that requires to be passed on kernel is contained in cmsg_hdr defined memory extension. So my understanding is the cmsghdr must be well formed where kernel can walk using the defined CMSG_* macros. >> It needs to be cleared because POSIX states the cmsg_len to be a socklen_t >> size, which for 64-bits architecture on Linux is still 32-bit unsigned >> types. So to be portable, programs can not rely on passing cmsghdr >> larger than socketlen_t size. > > This doesn't answer my question, but I think what you're saying is that the kernel defines cmsg_len as size_t even though POSIX specifies socklen_t, so you can have the application fill in only the low 32 bits but the kernel reads 64. Thing is, glibc shouldn't be trying to paper that over. It is more important for the C library to faithfully reflect what the kernel interface really is, than for it to check this particular conformance box. > GLIBC general position is to follow POSIX standard to provide a portable interface and I do not see a compelling reason to not do so in this particular case. Limiting cmsg_len to 32-bits is still a quite large buffer limit size and I doubt very much if any application is or will exceed this size anytime soon. Also, limiting the size does not compromise the interface since program may issue two or more syscalls if the used cmsghdr buffer does not fit in a socklen_t (as for every other socket call). > zw >
diff --git a/ChangeLog b/ChangeLog index ecedf6f..317d228 100644 --- a/ChangeLog +++ b/ChangeLog @@ -9,6 +9,9 @@ 2016-05-25 Adhemerval Zanella <adhemerval.zanella@linaro.org> + * sysdeps/unix/sysv/linux/sendmsg.c (__libc_sendmsg): Fix cmsghdr + padding for 64-bits. + * sysdeps/unix/sysv/linux/Makefile [$(subdir) = socket] (sysdep_routines): Add oldrecvmmsg and oldsendmmsg. diff --git a/sysdeps/unix/sysv/linux/sendmsg.c b/sysdeps/unix/sysv/linux/sendmsg.c index a5ef238..4a5420a 100644 --- a/sysdeps/unix/sysv/linux/sendmsg.c +++ b/sysdeps/unix/sysv/linux/sendmsg.c @@ -19,6 +19,7 @@ #include <sysdep-cancel.h> #include <socketcall.h> #include <shlib-compat.h> +#include <string.h> ssize_t __libc_sendmsg (int fd, const struct msghdr *msg, int flags) @@ -28,13 +29,35 @@ __libc_sendmsg (int fd, const struct msghdr *msg, int flags) both size_t. So for 64-bit it requires some adjustments by copying to temporary header and zeroing the pad fields. */ #if __WORDSIZE == 64 +# define CMSGHDR_CONTROLLEN_MAX 2048 struct msghdr hdr; + struct cmsghdr auxcbuf[CMSGHDR_CONTROLLEN_MAX/sizeof(struct cmsghdr)+1]; if (msg != NULL) { hdr = *msg; hdr.__glibc_reserved1 = 0; hdr.__glibc_reserved2 = 0; msg = &hdr; + + if (hdr.msg_controllen > 0) + { + /* Since function contract does not allow modify the msg_controllen + in place it requires to copy on a temporary buffer to make the + adjustment required by POSIX. Current limit is quite arbitrary + and it is expected to cover the mostly common user cases for + this (passing file descriptors and permission between processes + over unix sockets). */ + if (hdr.msg_controllen > CMSGHDR_CONTROLLEN_MAX) + { + __set_errno (ENOMEM); + return -1; + } + memcpy (auxcbuf, hdr.msg_control, hdr.msg_controllen); + hdr.msg_control = auxcbuf; + for (struct cmsghdr *c = CMSG_FIRSTHDR (&hdr); c != NULL; + c = CMSG_NXTHDR(&hdr, c)) + c->__glibc_reserved1 = 0; + } } #endif