diff mbox

network: Fix cmsghdr padding in sendmsg (BZ#16919)

Message ID 1464273450-31507-1-git-send-email-adhemerval.zanella@linaro.org
State New
Headers show

Commit Message

Adhemerval Zanella May 26, 2016, 2:37 p.m. UTC
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.

Tested on x86_64 and i686.

	* sysdeps/unix/sysv/linux/sendmsg.c (__libc_sendmsg): Fix cmsghdr
	padding for 64-bits.
---
 ChangeLog                         |  3 +++
 sysdeps/unix/sysv/linux/sendmsg.c | 23 +++++++++++++++++++++++
 2 files changed, 26 insertions(+)

-- 
2.7.4

Comments

Adhemerval Zanella May 26, 2016, 7:03 p.m. UTC | #1
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.
Adhemerval Zanella May 26, 2016, 7:46 p.m. UTC | #2
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

>
Adhemerval Zanella May 26, 2016, 9:42 p.m. UTC | #3
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 mbox

Patch

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