Fwd: Cross thread shutdown of connected UDP socket, unexpected recvfrom behavior

Fwd: Cross thread shutdown of connected UDP socket, unexpected recvfrom behavior

[mpb]

[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]

Hi netdev@vger.kernel.org,

Someone on LKML suggested that I forward this issue to the netdev list.

In a nutshell, recvfrom returns ambiguous results, and it is
impossible to distinguish between these two situations:

a) calling recvfrom and receiving a zero length UDP message
b) calling recvfrom and blocking until the socket is shutdown (locally)

More details below.

Thanks!

-mpb


---------- Forwarded message ----------
From: mpb <mpb.mail@gmail.com>
Date: Sat, Nov 9, 2013 at 5:32 PM
Subject: Cross thread shutdown of connected UDP socket, unexpected
recvfrom behavior
To: linux-kernel@vger.kernel.org


Hi LKML,

I have a C/pthreads program with two threads ("main" and "thread").
"thread" calls recvfrom on a connected UDP socket and blocks, waiting
for input.  "main" then calls shutdown SHUT_RD on the socket.  In
"thread", recvfrom returns, apparently successfully, returning a zero
length string, and setting src_addr to a bogus(?) address family,
port, and source address.

Is the above the correct behavior for recvfrom?

Here is output from my program.  "main" sends "hello\0", then "" (the
empty string), then calls shutdown.  "thread" receives "hello\0", "",
and then finally receives an empty string that was never sent!

thread  recvfrom: Success
  rv 6  addrlen 16  fam 2  port 8000  addr 100007f

thread  recvfrom: Success
  rv 0  addrlen 16  fam 2  port 8000  addr 100007f

main    shutdown: Success
  rv 0

thread  recvfrom: Success
  rv 0  addrlen 16  fam 59060  port 44237  addr deaadef0

The source code (2k) for the porgram is attached.  I'm running Ubuntu
13.04, kernel 3.8.0-19-generic #30-Ubuntu, on 32-bit i686.

For reference, this June 2000 LKML thread discusses calling close in a
similar situation.
http://www.gossamer-threads.com/lists/linux/kernel/144379

Please CC me if you have questions, otherwise I'll try to watch for
answers in the list archives.

Thanks!

-mpb

[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 1861 bytes --]



#include <netinet/in.h>
#include <pthread.h>
#include <stdio.h>
#include <sys/socket.h>
#include <sys/types.h>


struct sockaddr_in  addr_0, addr_1, addr_2 ;
int                 fd0, fd1, rv0, rv1, addrlen;
char                buf_0[1024], buf_1[1024];
pthread_t           thread;
void*               thread_rv;


void* thread_main (void* arg) {

  int i;  for ( i=0;  i<3;  i++ ) {

    addr_2.sin_family       =  0;
    addr_2.sin_port         =  0;
    addr_2.sin_addr.s_addr  =  0;

    addrlen = sizeof addr_2;
    rv1 = recvfrom ( fd1, buf_0, sizeof buf_0, 0,
		     (struct sockaddr*) &addr_2, &addrlen );

    int fam   =  addr_2.sin_family;
    int port  =  addr_2.sin_port;
    int addr  =  addr_2.sin_addr.s_addr;

    printf ("\n");
    perror ("thread  recvfrom");
    printf ( "  rv %d  addrlen %d  fam %d  port %d  addr %x\n",
	     rv1, addrlen, fam, ntohs (port), addr );
    ;;; }

  return NULL; }


void main_main () {

  send ( fd0, "hello", 6, 0);
  send ( fd0, "",      0, 0);
  usleep (100000);

  rv0 = shutdown ( fd1, SHUT_RD );
  printf ("\n");
  perror ("main    shutdown");
  printf ("  %d\n", rv0)

  ;;; }


int main () {

  addr_0.sin_family = AF_INET;
  addr_0.sin_port = htons (8000);
  inet_pton ( AF_INET, "127.0.0.1", &addr_0.sin_addr );

  addr_1.sin_family = AF_INET;
  addr_1.sin_port = htons (8001);
  inet_pton ( AF_INET, "127.0.0.1", &addr_1.sin_addr );

  fd0 = socket ( AF_INET, SOCK_DGRAM, 0 );
  bind    ( fd0, (struct sockaddr*) &addr_0, sizeof addr_0 );
  connect ( fd0, (struct sockaddr*) &addr_1, sizeof addr_1 );

  fd1 = socket ( AF_INET, SOCK_DGRAM, 0 );
  bind    ( fd1, (struct sockaddr*) &addr_1, sizeof addr_1 );
  connect ( fd1, (struct sockaddr*) &addr_0, sizeof addr_0 );

  pthread_create ( &thread, NULL, thread_main, NULL );
  main_main ();
  pthread_join ( thread, &thread_rv );

  return 0; }

Re: Fwd: Cross thread shutdown of connected UDP socket, unexpected recvfrom behavior

[Hannes Frederic Sowa]

On Wed, Nov 13, 2013 at 12:07:20PM -0800, mpb wrote:
> I have a C/pthreads program with two threads ("main" and "thread").
> "thread" calls recvfrom on a connected UDP socket and blocks, waiting
> for input.  "main" then calls shutdown SHUT_RD on the socket.  In
> "thread", recvfrom returns, apparently successfully, returning a zero
> length string, and setting src_addr to a bogus(?) address family,
> port, and source address.
> 
> Is the above the correct behavior for recvfrom?
> 
> Here is output from my program.  "main" sends "hello\0", then "" (the
> empty string), then calls shutdown.  "thread" receives "hello\0", "",
> and then finally receives an empty string that was never sent!
> 
> thread  recvfrom: Success
>   rv 6  addrlen 16  fam 2  port 8000  addr 100007f
> 
> thread  recvfrom: Success
>   rv 0  addrlen 16  fam 2  port 8000  addr 100007f
> 
> main    shutdown: Success
>   rv 0
> 
> thread  recvfrom: Success
>   rv 0  addrlen 16  fam 59060  port 44237  addr deaadef0
> 
> The source code (2k) for the porgram is attached.  I'm running Ubuntu
> 13.04, kernel 3.8.0-19-generic #30-Ubuntu, on 32-bit i686.
> 
> For reference, this June 2000 LKML thread discusses calling close in a
> similar situation.
> http://www.gossamer-threads.com/lists/linux/kernel/144379
> 
> Please CC me if you have questions, otherwise I'll try to watch for
> answers in the list archives.

Changing such errno values is often brittle e.g. returning -EPIPE instead of
an EOF marker. It is coded very demonstrative to not return an error in
wait_for_more_packets, so I guess there will be a reason.

But at least we should not return uninitialized data from the stack:

diff --git a/net/socket.c b/net/socket.c
index c226ace..44499db 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1834,6 +1834,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
 	if (!sock)
 		goto out;
 
+	memset(&address, 0, sizeof(address));
 	msg.msg_control = NULL;
 	msg.msg_controllen = 0;
 	msg.msg_iovlen = 1;
@@ -2228,6 +2229,8 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 
 	uaddr = (__force void __user *)msg_sys->msg_name;
 	uaddr_len = COMPAT_NAMELEN(msg);
+	if (uaddr != NULL)
+		memset(&addr, 0, sizeof(addr));
 	if (MSG_CMSG_COMPAT & flags) {
 		err = verify_compat_iovec(msg_sys, iov, &addr, VERIFY_WRITE);
 	} else


I'll test and submit the above patch if no one else speaks up.

Greetings,

  Hannes

[PATCH] socket: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

If a blocking read waits on a socket which gets concurrently shut down we
return 0 as error and so indicate success to the socket functions which
thus copy an uninitialized stack allocated address back to the user.
Fix this by clearing the 128 byte size (on x86-64) address first.

This patch fixes the problem for recvfrom, recvmsg and recvmmsg.

Reported-by: mpb <mpb.mail@gmail.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 net/socket.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index c226ace..44499db 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1834,6 +1834,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
 	if (!sock)
 		goto out;
 
+	memset(&address, 0, sizeof(address));
 	msg.msg_control = NULL;
 	msg.msg_controllen = 0;
 	msg.msg_iovlen = 1;
@@ -2228,6 +2229,8 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 
 	uaddr = (__force void __user *)msg_sys->msg_name;
 	uaddr_len = COMPAT_NAMELEN(msg);
+	if (uaddr != NULL)
+		memset(&addr, 0, sizeof(addr));
 	if (MSG_CMSG_COMPAT & flags) {
 		err = verify_compat_iovec(msg_sys, iov, &addr, VERIFY_WRITE);
 	} else
-- 
1.8.3.1

[PATCH v2] socket: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

If a blocking read waits on a socket which gets concurrently shut down we
return 0 as error and so indicate success to the socket functions which
thus copy an uninitialized stack allocated address back to the user.
Fix this by clearing the 128 byte size (on x86-64) address first.

This patch fixes the problem for recvfrom, recvmsg and recvmmsg.

Reported-by: mpb <mpb.mail@gmail.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
Sorry, grabbed the wrong patch file.

 net/socket.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/socket.c b/net/socket.c
index c226ace..a44f29c 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1834,6 +1834,8 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
 	if (!sock)
 		goto out;
 
+	if (addr != NULL)
+		memset(&address, 0, sizeof(address));
 	msg.msg_control = NULL;
 	msg.msg_controllen = 0;
 	msg.msg_iovlen = 1;
@@ -2228,6 +2230,8 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 
 	uaddr = (__force void __user *)msg_sys->msg_name;
 	uaddr_len = COMPAT_NAMELEN(msg);
+	if (uaddr != NULL)
+		memset(&addr, 0, sizeof(addr));
 	if (MSG_CMSG_COMPAT & flags) {
 		err = verify_compat_iovec(msg_sys, iov, &addr, VERIFY_WRITE);
 	} else
-- 
1.8.3.1

Re: [PATCH v2] socket: don't return uninitialized addresses on concurrent socket shutdown

[Eric Dumazet]

On Sat, 2013-11-16 at 06:48 +0100, Hannes Frederic Sowa wrote:
> If a blocking read waits on a socket which gets concurrently shut down we
> return 0 as error and so indicate success to the socket functions which
> thus copy an uninitialized stack allocated address back to the user.
> Fix this by clearing the 128 byte size (on x86-64) address first.
> 
> This patch fixes the problem for recvfrom, recvmsg and recvmmsg.
> 
> Reported-by: mpb <mpb.mail@gmail.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---

Are you clearing 128 bytes on every recvfrom() system call, just in case
of this shutdown() issue ?

Can't we avoid this overhead ?

msg.msg_namelen should be set to 0 in this case.

Re: [PATCH v2] socket: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

On Fri, Nov 15, 2013 at 10:32:49PM -0800, Eric Dumazet wrote:
> On Sat, 2013-11-16 at 06:48 +0100, Hannes Frederic Sowa wrote:
> > If a blocking read waits on a socket which gets concurrently shut down we
> > return 0 as error and so indicate success to the socket functions which
> > thus copy an uninitialized stack allocated address back to the user.
> > Fix this by clearing the 128 byte size (on x86-64) address first.
> > 
> > This patch fixes the problem for recvfrom, recvmsg and recvmmsg.
> > 
> > Reported-by: mpb <mpb.mail@gmail.com>
> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > ---
> 
> Are you clearing 128 bytes on every recvfrom() system call, just in case
> of this shutdown() issue ?

Yes, that gave me a bad feeling, too (so I explicitly mentioned it in the
changelog and hoped for some discussion).

> Can't we avoid this overhead ?
> 
> msg.msg_namelen should be set to 0 in this case.

I don't see how, currently. Either we tunnel a new return value through
->recvmsg or we use the address structure, mark it with a special AF_FOO and
check if we get back that same value.

I don't see how msg.msg_namelen set to zero can help?

Re: [PATCH v2] socket: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

On Sat, Nov 16, 2013 at 07:39:56AM +0100, Hannes Frederic Sowa wrote:
> On Fri, Nov 15, 2013 at 10:32:49PM -0800, Eric Dumazet wrote:
> > On Sat, 2013-11-16 at 06:48 +0100, Hannes Frederic Sowa wrote:
> > > If a blocking read waits on a socket which gets concurrently shut down we
> > > return 0 as error and so indicate success to the socket functions which
> > > thus copy an uninitialized stack allocated address back to the user.
> > > Fix this by clearing the 128 byte size (on x86-64) address first.
> > > 
> > > This patch fixes the problem for recvfrom, recvmsg and recvmmsg.
> > > 
> > > Reported-by: mpb <mpb.mail@gmail.com>
> > > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > > ---
> > 
> > Are you clearing 128 bytes on every recvfrom() system call, just in case
> > of this shutdown() issue ?
> 
> Yes, that gave me a bad feeling, too (so I explicitly mentioned it in the
> changelog and hoped for some discussion).
> 
> > Can't we avoid this overhead ?
> > 
> > msg.msg_namelen should be set to 0 in this case.
> 
> I don't see how, currently. Either we tunnel a new return value through
> ->recvmsg or we use the address structure, mark it with a special AF_FOO and
> check if we get back that same value.
> 
> I don't see how msg.msg_namelen set to zero can help?

We could also think about changing the return value on concurrent closed
sockets entirely and avoiding this whole issue. It would only change
concurrent shutdown()/read() behaviour, not close as far as I can see.

[PATCH v3] net: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

If a blocking read waits on a socket which gets concurrently shut down we
return 0 as error and so indicate success to the socket functions which
thus copies an uninitialized stack allocated address back to the user.

Fix this by introducing a new AF_INVALID sa_family marker and check if the
recvmsg function overwrote it. In case it was not overwritten, clear the
address with zeros (AF_UNSPEC) before returning it to the user. IMHO we
should only increase msg.msg_namelen (if we have to truncate the address),
so don't clear msg.msg_namelen.

This patch fixes the problem for recvfrom, recvmsg and recvmmsg.

Reported-by: mpb <mpb.mail@gmail.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
 include/linux/socket.h | 2 ++
 net/socket.c           | 6 ++++++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/socket.h b/include/linux/socket.h
index 445ef75..bdf9205 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -182,6 +182,8 @@ struct ucred {
 #define AF_VSOCK	40	/* vSockets			*/
 #define AF_MAX		41	/* For now.. */
 
+#define AF_INVALID	((__kernel_sa_family_t)(~0U))
+
 /* Protocol families, same as address families. */
 #define PF_UNSPEC	AF_UNSPEC
 #define PF_UNIX		AF_UNIX
diff --git a/net/socket.c b/net/socket.c
index c226ace..8361e15 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1834,6 +1834,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
 	if (!sock)
 		goto out;
 
+	address.ss_family = AF_INVALID;
 	msg.msg_control = NULL;
 	msg.msg_controllen = 0;
 	msg.msg_iovlen = 1;
@@ -1847,6 +1848,8 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
 	err = sock_recvmsg(sock, &msg, size, flags);
 
 	if (err >= 0 && addr != NULL) {
+		if (unlikely(address.ss_family == AF_INVALID))
+			memset(&address, 0, sizeof(address));
 		err2 = move_addr_to_user(&address,
 					 msg.msg_namelen, addr, addr_len);
 		if (err2 < 0)
@@ -2226,6 +2229,7 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 	 *      kernel msghdr to use the kernel address space)
 	 */
 
+	addr.ss_family = AF_INVALID;
 	uaddr = (__force void __user *)msg_sys->msg_name;
 	uaddr_len = COMPAT_NAMELEN(msg);
 	if (MSG_CMSG_COMPAT & flags) {
@@ -2248,6 +2252,8 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 	len = err;
 
 	if (uaddr != NULL) {
+		if (unlikely(addr.ss_family == AF_INVALID))
+			memset(&addr, 0, sizeof(addr));
 		err = move_addr_to_user(&addr,
 					msg_sys->msg_namelen, uaddr,
 					uaddr_len);
-- 
1.8.3.1

Re: [PATCH v3] net: don't return uninitialized addresses on concurrent socket shutdown

[Eric Dumazet]

On Sat, 2013-11-16 at 20:19 +0100, Hannes Frederic Sowa wrote:
> If a blocking read waits on a socket which gets concurrently shut down we
> return 0 as error and so indicate success to the socket functions which
> thus copies an uninitialized stack allocated address back to the user.
> 
> Fix this by introducing a new AF_INVALID sa_family marker and check if the
> recvmsg function overwrote it. In case it was not overwritten, clear the
> address with zeros (AF_UNSPEC) before returning it to the user. IMHO we
> should only increase msg.msg_namelen (if we have to truncate the address),
> so don't clear msg.msg_namelen.
> 
> This patch fixes the problem for recvfrom, recvmsg and recvmmsg.
> 
> Reported-by: mpb <mpb.mail@gmail.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
>  include/linux/socket.h | 2 ++
>  net/socket.c           | 6 ++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index 445ef75..bdf9205 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -182,6 +182,8 @@ struct ucred {
>  #define AF_VSOCK	40	/* vSockets			*/
>  #define AF_MAX		41	/* For now.. */
>  
> +#define AF_INVALID	((__kernel_sa_family_t)(~0U))
> +
>  /* Protocol families, same as address families. */
>  #define PF_UNSPEC	AF_UNSPEC
>  #define PF_UNIX		AF_UNIX
> diff --git a/net/socket.c b/net/socket.c
> index c226ace..8361e15 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1834,6 +1834,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
>  	if (!sock)
>  		goto out;
>  
> +	address.ss_family = AF_INVALID;


	address.ss_family = AF_UNSPEC;

>  	msg.msg_control = NULL;
>  	msg.msg_controllen = 0;
>  	msg.msg_iovlen = 1;
> @@ -1847,6 +1848,8 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
>  	err = sock_recvmsg(sock, &msg, size, flags);
>  
>  	if (err >= 0 && addr != NULL) {
> +		if (unlikely(address.ss_family == AF_INVALID))
> +			memset(&address, 0, sizeof(address));

Why clearing 128 bytes, and return msg.msg_namelen null bytes to the
user ?

What useful information will the user get from this ? Is this even
documented ?


>  		err2 = move_addr_to_user(&address,
>  					 msg.msg_namelen, addr, addr_len);
>  		if (err2 < 0)

I really don't think userland should expect to read 128 null bytes if it
asked 128 bytes.

We can certainly return 2 null bytes (AF_UNSPEC) and comply with the
documentation.

if (unlikely(address.ss_family == AF_UNSPEC))
	msg.msg_namelen = sizeof(address.ss_family);

Re: [PATCH v3] net: don't return uninitialized addresses on concurrent socket shutdown

[mpb]

>> @@ -1847,6 +1848,8 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
>>       err = sock_recvmsg(sock, &msg, size, flags);
>>
>>       if (err >= 0 && addr != NULL) {
>> +             if (unlikely(address.ss_family == AF_INVALID))
>> +                     memset(&address, 0, sizeof(address));

Or perhaps only when err == 0?

      if (err >= 0 && addr != NULL) {
+             if ( (err == 0)  &&  unlikely(address.ss_family == AF_INVALID) )
+                     memset(&address, 0, sizeof(address));

In other words, in pseudo code:

When err == 0, the kernel checks to make sure the socket has not been
shutdown before calling move_addr_to_user.
If err == 0 and a shutdown has happened, then set addr to a value that
signifies that a shutdown was detected (or at least that a message was
not received).  After a shutdown, I believe err will only be one of 0
or -1.  If err > 0, then a shutdown has not happened.

Another approach would be to return -1 to userland and set errno to
something like EWOULDBLOCK.

Another thought: addr_len is probably smaller than sizeof(address), so
only memset whichever is smaller.

If I had a *BSD box or VM sitting around, I would look to see what
*BSD does (and perhaps copy the *BSD behavior if it were acceptable).

Eric Dumazet wrote:
> I really don't think userland should expect to read 128 null bytes if it
> asked 128 bytes.
>
> We can certainly return 2 null bytes (AF_UNSPEC)

AF_UNSPEC is good (and sufficient, IMO).

Do we need to add AF_INVALID at all?  Why not just use AF_UNSPEC?

> and comply with the documentation.

What documentation?  The interesting thing about this situation is I
have not seen any manpage that describes what should happen after a
concurrent shutdown.

-mpb

Re: [PATCH v3] net: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

On Sat, Nov 16, 2013 at 12:46:42PM -0800, Eric Dumazet wrote:
> On Sat, 2013-11-16 at 20:19 +0100, Hannes Frederic Sowa wrote:
> > If a blocking read waits on a socket which gets concurrently shut down we
> > return 0 as error and so indicate success to the socket functions which
> > thus copies an uninitialized stack allocated address back to the user.
> > 
> > Fix this by introducing a new AF_INVALID sa_family marker and check if the
> > recvmsg function overwrote it. In case it was not overwritten, clear the
> > address with zeros (AF_UNSPEC) before returning it to the user. IMHO we
> > should only increase msg.msg_namelen (if we have to truncate the address),
> > so don't clear msg.msg_namelen.
> > 
> > This patch fixes the problem for recvfrom, recvmsg and recvmmsg.
> > 
> > Reported-by: mpb <mpb.mail@gmail.com>
> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > ---
> >  include/linux/socket.h | 2 ++
> >  net/socket.c           | 6 ++++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/include/linux/socket.h b/include/linux/socket.h
> > index 445ef75..bdf9205 100644
> > --- a/include/linux/socket.h
> > +++ b/include/linux/socket.h
> > @@ -182,6 +182,8 @@ struct ucred {
> >  #define AF_VSOCK	40	/* vSockets			*/
> >  #define AF_MAX		41	/* For now.. */
> >  
> > +#define AF_INVALID	((__kernel_sa_family_t)(~0U))
> > +
> >  /* Protocol families, same as address families. */
> >  #define PF_UNSPEC	AF_UNSPEC
> >  #define PF_UNIX		AF_UNIX
> > diff --git a/net/socket.c b/net/socket.c
> > index c226ace..8361e15 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1834,6 +1834,7 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
> >  	if (!sock)
> >  		goto out;
> >  
> > +	address.ss_family = AF_INVALID;
> 
> 
> 	address.ss_family = AF_UNSPEC;

That might be possible. Because I was afraid that AF_UNSPEC could re misused
in some other protocols I decided to use the (what I think) more robust
AF_INVALID.

> 
> >  	msg.msg_control = NULL;
> >  	msg.msg_controllen = 0;
> >  	msg.msg_iovlen = 1;
> > @@ -1847,6 +1848,8 @@ SYSCALL_DEFINE6(recvfrom, int, fd, void __user *, ubuf, size_t, size,
> >  	err = sock_recvmsg(sock, &msg, size, flags);
> >  
> >  	if (err >= 0 && addr != NULL) {
> > +		if (unlikely(address.ss_family == AF_INVALID))
> > +			memset(&address, 0, sizeof(address));
> 
> Why clearing 128 bytes, and return msg.msg_namelen null bytes to the
> user ?
> 
> What useful information will the user get from this ? Is this even
> documented ?
> 
> 
> >  		err2 = move_addr_to_user(&address,
> >  					 msg.msg_namelen, addr, addr_len);
> >  		if (err2 < 0)
> 
> I really don't think userland should expect to read 128 null bytes if it
> asked 128 bytes.
> 
> We can certainly return 2 null bytes (AF_UNSPEC) and comply with the
> documentation.
> 
> if (unlikely(address.ss_family == AF_UNSPEC))
> 	msg.msg_namelen = sizeof(address.ss_family);

I have thought about that:

I do think it is common to call recvfrom, process the packet and sendto
back a packet with the updated values from recvfrom. We accept AF_UNSPEC
on an IPv4 UDP socket and use the addresses as it would be a AF_INET
sockaddr. We only bail out if the port is 0.

It was my intend to at least clear the addressing portions of the regular
sockaddr_* structure for the user as it could be reused as explained
earlier and be allocated uninitialized on the stack (or reused, so
sending packet to a previous destination). I think it is very uncommon to
expect a non-error value on a recvfrom/recvmsg and have AF_UNSPEC in the
sockaddr.

(I erroneously stated that we could return the full 128 zero bytes, we only
clear 128 bytes and return only max(128, msg.msg_namelen). msg_namelen gets
updated by the recvmsg handler and that only iff we have this concurrent
shutdown and blocking read issue.)

If the socket structure is cleared a following sendto would produce a -EINVAL.

Maybe I am too sensible regarding such problems and will think about that a
bit more (and check for AF_INVALID/AF_UNSPEC).

Thanks,

  Hannes

Re: [PATCH v3] net: don't return uninitialized addresses on concurrent socket shutdown

[Eric Dumazet]

On Sat, 2013-11-16 at 23:43 +0100, Hannes Frederic Sowa wrote:

> I do think it is common to call recvfrom, process the packet and sendto
> back a packet with the updated values from recvfrom. We accept AF_UNSPEC
> on an IPv4 UDP socket and use the addresses as it would be a AF_INET
> sockaddr. We only bail out if the port is 0.
> 
> It was my intend to at least clear the addressing portions of the regular
> sockaddr_* structure for the user as it could be reused as explained
> earlier and be allocated uninitialized on the stack (or reused, so
> sending packet to a previous destination). I think it is very uncommon to
> expect a non-error value on a recvfrom/recvmsg and have AF_UNSPEC in the
> sockaddr.
> 
> (I erroneously stated that we could return the full 128 zero bytes, we only
> clear 128 bytes and return only max(128, msg.msg_namelen). msg_namelen gets
> updated by the recvmsg handler and that only iff we have this concurrent
> shutdown and blocking read issue.)
> 
> If the socket structure is cleared a following sendto would produce a -EINVAL.
> 
> Maybe I am too sensible regarding such problems and will think about that a
> bit more (and check for AF_INVALID/AF_UNSPEC).
> 


I think the _default_ should be to clear it.

-	msg.msg_namelen = sizeof(address);
+	msg.msg_namelen = 0;

And subsystems filling a real address would set it back to the length
they took care of.

in recvfrom() paths, the kernel _knows_ it uses an array of 128 bytes.
(struct sockaddr_storage)

Re: [PATCH v3] net: don't return uninitialized addresses on concurrent socket shutdown

[Hannes Frederic Sowa]

On Sat, Nov 16, 2013 at 04:36:24PM -0800, Eric Dumazet wrote:
> On Sat, 2013-11-16 at 23:43 +0100, Hannes Frederic Sowa wrote:
> 
> > I do think it is common to call recvfrom, process the packet and sendto
> > back a packet with the updated values from recvfrom. We accept AF_UNSPEC
> > on an IPv4 UDP socket and use the addresses as it would be a AF_INET
> > sockaddr. We only bail out if the port is 0.
> > 
> > It was my intend to at least clear the addressing portions of the regular
> > sockaddr_* structure for the user as it could be reused as explained
> > earlier and be allocated uninitialized on the stack (or reused, so
> > sending packet to a previous destination). I think it is very uncommon to
> > expect a non-error value on a recvfrom/recvmsg and have AF_UNSPEC in the
> > sockaddr.
> > 
> > (I erroneously stated that we could return the full 128 zero bytes, we only
> > clear 128 bytes and return only max(128, msg.msg_namelen). msg_namelen gets
> > updated by the recvmsg handler and that only iff we have this concurrent
> > shutdown and blocking read issue.)
> > 
> > If the socket structure is cleared a following sendto would produce a -EINVAL.
> > 
> > Maybe I am too sensible regarding such problems and will think about that a
> > bit more (and check for AF_INVALID/AF_UNSPEC).
> > 
> 
> I think the _default_ should be to clear it.
> 
> -	msg.msg_namelen = sizeof(address);
> +	msg.msg_namelen = 0;
> 
> And subsystems filling a real address would set it back to the length
> they took care of.
> 
> in recvfrom() paths, the kernel _knows_ it uses an array of 128 bytes.
> (struct sockaddr_storage)

I agree, that's the only sane approach we can take. Both the AF_UNSPEC and
also the AF_INVALID approach does conflict how packet sockets structure their
sockaddr_pkt (dev->type is put in sa_family and both 0 and 0xFFFF are valid
interface types).

I'll check it out.

Thanks,

  Hannes

[PATCH v4] inet: prevent leakage of uninitialized memory to user in recv syscalls

[Hannes Frederic Sowa]

Only update *addr_len when we actually fill in sockaddr, otherwise we
can return uninitialized memory from the stack to the caller in the
recvfrom, recvmmsg and recvmsg syscalls. Drop the the (addr_len == NULL)
checks because we only get called with a valid addr_len pointer either
from sock_common_recvmsg or inet_recvmsg.

If a blocking read waits on a socket which is concurrently shut down we
now return zero and set msg_msgnamelen to 0.

Reported-by: mpb <mpb.mail@gmail.com>
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
This is the first round, fixing the callees of sock_common_recvmsg and
inet_recvmsg. I guess there are some more problems of this kind with
recvmsg called directly via proto_ops->recvmsg. I will have a look at
them in the next days.

We could actually leave out filling in msg_name if the user didn't request
(src_addr == NULL in recvfrom). I'll put this on my TODO for net-next.
Also it is possible to just update msg->msg_namelen directly here and simplify
sock_common_recvmsg and inet_recvmsg, also for net-next.

Greetings,

  Hannes

 net/ieee802154/dgram.c |  3 +--
 net/ipv4/ping.c        | 19 +++++++------------
 net/ipv4/raw.c         |  4 +---
 net/ipv4/udp.c         |  7 +------
 net/ipv6/raw.c         |  4 +---
 net/ipv6/udp.c         |  5 +----
 net/l2tp/l2tp_ip.c     |  4 +---
 net/phonet/datagram.c  |  9 ++++-----
 8 files changed, 17 insertions(+), 38 deletions(-)

diff --git a/net/ieee802154/dgram.c b/net/ieee802154/dgram.c
index 581a595..1865fdf 100644
--- a/net/ieee802154/dgram.c
+++ b/net/ieee802154/dgram.c
@@ -315,9 +315,8 @@ static int dgram_recvmsg(struct kiocb *iocb, struct sock *sk,
 	if (saddr) {
 		saddr->family = AF_IEEE802154;
 		saddr->addr = mac_cb(skb)->sa;
-	}
-	if (addr_len)
 		*addr_len = sizeof(*saddr);
+	}
 
 	if (flags & MSG_TRUNC)
 		copied = skb->len;
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 9afbdb1..aacefa0 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -830,8 +830,6 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 {
 	struct inet_sock *isk = inet_sk(sk);
 	int family = sk->sk_family;
-	struct sockaddr_in *sin;
-	struct sockaddr_in6 *sin6;
 	struct sk_buff *skb;
 	int copied, err;
 
@@ -841,13 +839,6 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	if (flags & MSG_OOB)
 		goto out;
 
-	if (addr_len) {
-		if (family == AF_INET)
-			*addr_len = sizeof(*sin);
-		else if (family == AF_INET6 && addr_len)
-			*addr_len = sizeof(*sin6);
-	}
-
 	if (flags & MSG_ERRQUEUE) {
 		if (family == AF_INET) {
 			return ip_recv_error(sk, msg, len);
@@ -877,11 +868,13 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 
 	/* Copy the address and add cmsg data. */
 	if (family == AF_INET) {
-		sin = (struct sockaddr_in *) msg->msg_name;
+		struct sockaddr_in *sin = (struct sockaddr_in *)msg->msg_name;
+
 		sin->sin_family = AF_INET;
 		sin->sin_port = 0 /* skb->h.uh->source */;
 		sin->sin_addr.s_addr = ip_hdr(skb)->saddr;
 		memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
+		*addr_len = sizeof(*sin);
 
 		if (isk->cmsg_flags)
 			ip_cmsg_recv(msg, skb);
@@ -890,17 +883,19 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	} else if (family == AF_INET6) {
 		struct ipv6_pinfo *np = inet6_sk(sk);
 		struct ipv6hdr *ip6 = ipv6_hdr(skb);
-		sin6 = (struct sockaddr_in6 *) msg->msg_name;
+		struct sockaddr_in6 *sin6 =
+			(struct sockaddr_in6 *)msg->msg_name;
+
 		sin6->sin6_family = AF_INET6;
 		sin6->sin6_port = 0;
 		sin6->sin6_addr = ip6->saddr;
-
 		sin6->sin6_flowinfo = 0;
 		if (np->sndflow)
 			sin6->sin6_flowinfo = ip6_flowinfo(ip6);
 
 		sin6->sin6_scope_id = ipv6_iface_scope_id(&sin6->sin6_addr,
 							  IP6CB(skb)->iif);
+		*addr_len = sizeof(*sin6);
 
 		if (inet6_sk(sk)->rxopt.all)
 			pingv6_ops.ip6_datagram_recv_ctl(sk, msg, skb);
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 41e1d28..5cb8ddb 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -696,9 +696,6 @@ static int raw_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	if (flags & MSG_OOB)
 		goto out;
 
-	if (addr_len)
-		*addr_len = sizeof(*sin);
-
 	if (flags & MSG_ERRQUEUE) {
 		err = ip_recv_error(sk, msg, len);
 		goto out;
@@ -726,6 +723,7 @@ static int raw_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 		sin->sin_addr.s_addr = ip_hdr(skb)->saddr;
 		sin->sin_port = 0;
 		memset(&sin->sin_zero, 0, sizeof(sin->sin_zero));
+		*addr_len = sizeof(*sin);
 	}
 	if (inet->cmsg_flags)
 		ip_cmsg_recv(msg, skb);
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 89909dd..998431c 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1235,12 +1235,6 @@ int udp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
 	int is_udplite = IS_UDPLITE(sk);
 	bool slow;
 
-	/*
-	 *	Check any passed addresses
-	 */
-	if (addr_len)
-		*addr_len = sizeof(*sin);
-
 	if (flags & MSG_ERRQUEUE)
 		return ip_recv_error(sk, msg, len);
 
@@ -1302,6 +1296,7 @@ try_again:
 		sin->sin_port = udp_hdr(skb)->source;
 		sin->sin_addr.s_addr = ip_hdr(skb)->saddr;
 		memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
+		*addr_len = sizeof(*sin);
 	}
 	if (inet->cmsg_flags)
 		ip_cmsg_recv(msg, skb);
diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c
index 3c00842..e24ff1d 100644
--- a/net/ipv6/raw.c
+++ b/net/ipv6/raw.c
@@ -465,9 +465,6 @@ static int rawv6_recvmsg(struct kiocb *iocb, struct sock *sk,
 	if (flags & MSG_OOB)
 		return -EOPNOTSUPP;
 
-	if (addr_len)
-		*addr_len=sizeof(*sin6);
-
 	if (flags & MSG_ERRQUEUE)
 		return ipv6_recv_error(sk, msg, len);
 
@@ -506,6 +503,7 @@ static int rawv6_recvmsg(struct kiocb *iocb, struct sock *sk,
 		sin6->sin6_flowinfo = 0;
 		sin6->sin6_scope_id = ipv6_iface_scope_id(&sin6->sin6_addr,
 							  IP6CB(skb)->iif);
+		*addr_len = sizeof(*sin6);
 	}
 
 	sock_recv_ts_and_drops(msg, sk, skb);
diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index f3893e8..81eb8cf 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -392,9 +392,6 @@ int udpv6_recvmsg(struct kiocb *iocb, struct sock *sk,
 	int is_udp4;
 	bool slow;
 
-	if (addr_len)
-		*addr_len = sizeof(struct sockaddr_in6);
-
 	if (flags & MSG_ERRQUEUE)
 		return ipv6_recv_error(sk, msg, len);
 
@@ -480,7 +477,7 @@ try_again:
 				ipv6_iface_scope_id(&sin6->sin6_addr,
 						    IP6CB(skb)->iif);
 		}
-
+		*addr_len = sizeof(*sin6);
 	}
 	if (is_udp4) {
 		if (inet->cmsg_flags)
diff --git a/net/l2tp/l2tp_ip.c b/net/l2tp/l2tp_ip.c
index 571db8d..da1a1ce 100644
--- a/net/l2tp/l2tp_ip.c
+++ b/net/l2tp/l2tp_ip.c
@@ -518,9 +518,6 @@ static int l2tp_ip_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *m
 	if (flags & MSG_OOB)
 		goto out;
 
-	if (addr_len)
-		*addr_len = sizeof(*sin);
-
 	skb = skb_recv_datagram(sk, flags, noblock, &err);
 	if (!skb)
 		goto out;
@@ -543,6 +540,7 @@ static int l2tp_ip_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *m
 		sin->sin_addr.s_addr = ip_hdr(skb)->saddr;
 		sin->sin_port = 0;
 		memset(&sin->sin_zero, 0, sizeof(sin->sin_zero));
+		*addr_len = sizeof(*sin);
 	}
 	if (inet->cmsg_flags)
 		ip_cmsg_recv(msg, skb);
diff --git a/net/phonet/datagram.c b/net/phonet/datagram.c
index 12c30f3..38946b2 100644
--- a/net/phonet/datagram.c
+++ b/net/phonet/datagram.c
@@ -139,9 +139,6 @@ static int pn_recvmsg(struct kiocb *iocb, struct sock *sk,
 			MSG_CMSG_COMPAT))
 		goto out_nofree;
 
-	if (addr_len)
-		*addr_len = sizeof(sa);
-
 	skb = skb_recv_datagram(sk, flags, noblock, &rval);
 	if (skb == NULL)
 		goto out_nofree;
@@ -162,8 +159,10 @@ static int pn_recvmsg(struct kiocb *iocb, struct sock *sk,
 
 	rval = (flags & MSG_TRUNC) ? skb->len : copylen;
 
-	if (msg->msg_name != NULL)
-		memcpy(msg->msg_name, &sa, sizeof(struct sockaddr_pn));
+	if (msg->msg_name != NULL) {
+		memcpy(msg->msg_name, &sa, sizeof(sa));
+		*addr_len = sizeof(sa);
+	}
 
 out:
 	skb_free_datagram(sk, skb);
-- 
1.8.3.1

Re: [PATCH v4] inet: prevent leakage of uninitialized memory to user in recv syscalls

[Eric Dumazet]

On Mon, 2013-11-18 at 04:20 +0100, Hannes Frederic Sowa wrote:
> Only update *addr_len when we actually fill in sockaddr, otherwise we
> can return uninitialized memory from the stack to the caller in the
> recvfrom, recvmmsg and recvmsg syscalls. Drop the the (addr_len == NULL)
> checks because we only get called with a valid addr_len pointer either
> from sock_common_recvmsg or inet_recvmsg.
> 
> If a blocking read waits on a socket which is concurrently shut down we
> now return zero and set msg_msgnamelen to 0.
> 
> Reported-by: mpb <mpb.mail@gmail.com>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> This is the first round, fixing the callees of sock_common_recvmsg and
> inet_recvmsg. I guess there are some more problems of this kind with
> recvmsg called directly via proto_ops->recvmsg. I will have a look at
> them in the next days.
> 
> We could actually leave out filling in msg_name if the user didn't request
> (src_addr == NULL in recvfrom). I'll put this on my TODO for net-next.
> Also it is possible to just update msg->msg_namelen directly here and simplify
> sock_common_recvmsg and inet_recvmsg, also for net-next.

This definitely looks much better, as this will remove lot of crap,
thanks !

unix_dgram_recvmsg() and unix_stream_recvmsg() for example clear
msg_namelen

 

Re: [PATCH v4] inet: prevent leakage of uninitialized memory to user in recv syscalls

[David Miller]

From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Mon, 18 Nov 2013 04:20:45 +0100

> Only update *addr_len when we actually fill in sockaddr, otherwise we
> can return uninitialized memory from the stack to the caller in the
> recvfrom, recvmmsg and recvmsg syscalls. Drop the the (addr_len == NULL)
> checks because we only get called with a valid addr_len pointer either
> from sock_common_recvmsg or inet_recvmsg.
> 
> If a blocking read waits on a socket which is concurrently shut down we
> now return zero and set msg_msgnamelen to 0.
> 
> Reported-by: mpb <mpb.mail@gmail.com>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> ---
> This is the first round, fixing the callees of sock_common_recvmsg and
> inet_recvmsg. I guess there are some more problems of this kind with
> recvmsg called directly via proto_ops->recvmsg. I will have a look at
> them in the next days.
> 
> We could actually leave out filling in msg_name if the user didn't request
> (src_addr == NULL in recvfrom). I'll put this on my TODO for net-next.
> Also it is possible to just update msg->msg_namelen directly here and simplify
> sock_common_recvmsg and inet_recvmsg, also for net-next.

This looks a lot better than your previous attempts, nice work.

Applied and queued up for -stable.

The handling of msg_name/msg_namelen in the ->recvmsg proto op is decidedly
non-trivial.  Can I suggest that we at least add some documentation about
this semantic above "(*recvmsg)" in the definition of proto_ops in
include/linux/net.h?

Thanks.

Re: [PATCH v4] inet: prevent leakage of uninitialized memory to user in recv syscalls

[Hannes Frederic Sowa]

On Mon, Nov 18, 2013 at 08:13:50AM -0800, Eric Dumazet wrote:
> This definitely looks much better, as this will remove lot of crap,
> thanks !
> 
> unix_dgram_recvmsg() and unix_stream_recvmsg() for example clear
> msg_namelen

Most recvfrom calls are fine, but there is appletalk, tun and even macvtap
and a lot other recvmsg calls I still have to check.

On Mon, Nov 18, 2013 at 03:14:30PM -0500, David Miller wrote:
> This looks a lot better than your previous attempts, nice work.
> 
> Applied and queued up for -stable.
> 
> The handling of msg_name/msg_namelen in the ->recvmsg proto op is decidedly
> non-trivial.  Can I suggest that we at least add some documentation about
> this semantic above "(*recvmsg)" in the definition of proto_ops in
> include/linux/net.h?

Thanks!

I even want to come up with a better scheme for that. I do think we
can pass msg_namelen == 0 initially and let the recvmsg handlers update
it. So we have a safe fallback. The possible uninitialized memory leaks
can be pretty big, so I would like to see new recvmsg callers not return
anything if they don't care about msg_namelen. This should get noticed
during testing.

I'll add documentation in future patches.