netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* question about klen in move_addr_to_user()
@ 2013-03-18 10:10 Dan Carpenter
  2013-03-19 13:55 ` David Miller
  0 siblings, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2013-03-18 10:10 UTC (permalink / raw)
  To: netdev

Smatch complains that about a potential buffer overflow in
move_add_to_user()

net/socket.c
   212  static int move_addr_to_user(struct sockaddr_storage *kaddr, int klen,
   213                               void __user *uaddr, int __user *ulen)
   214  {
   215          int err;
   216          int len;
   217  
   218          err = get_user(len, ulen);
   219          if (err)
   220                  return err;
   221          if (len > klen)
   222                  len = klen;
   223          if (len < 0 || len > sizeof(struct sockaddr_storage))
   224                  return -EINVAL;
   225          if (len) {
   226                  if (audit_sockaddr(klen, kaddr))
                                           ^^^^
Smatch complains that although "len" is capped here, "klen" hasn't
necessarily been.  If "klen" is more than 128 bytes it leads to
memory corruption.

   227                          return -ENOMEM;
   228                  if (copy_to_user(uaddr, kaddr, len))
   229                          return -EFAULT;
   230          }

The call tree is this:

__sys_recvmsg() gets the msg->msg_namelen from the user.

Normally the network protocols set msg->msg_namelen in their
->recvmsg() function but some don't like caif_seqpkt_recvmsg() and
recv_msg() for tipc.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: question about klen in move_addr_to_user()
  2013-03-18 10:10 question about klen in move_addr_to_user() Dan Carpenter
@ 2013-03-19 13:55 ` David Miller
  2013-10-02 18:58   ` [patch] net: heap overflow in __audit_sockaddr() Dan Carpenter
  0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2013-03-19 13:55 UTC (permalink / raw)
  To: dan.carpenter; +Cc: netdev

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Mon, 18 Mar 2013 13:10:07 +0300

> The call tree is this:
> 
> __sys_recvmsg() gets the msg->msg_namelen from the user.
> 
> Normally the network protocols set msg->msg_namelen in their
> ->recvmsg() function but some don't like caif_seqpkt_recvmsg() and
> recv_msg() for tipc.

In fact, even TCP will just leave the msg->msg_namelen alone.

I think the best thing to do is to cap the klen to the size of
sockaddr_storage in verify_iovec() when mode is not VERIFY_READ.

But actually, it looks like sendmsg() has a similar problem.
We use m->msg_namelen as-is in verify_iovec() via __sys_sendmsg()
when mode is VERIFY_READ.

This makes me think that we should cap this at the precise moment
we import the user's msghdr.  Which means:

1) Create a helper function copy_msghdr_from_user() and use
   it everywhere we do the straight copy_from_user(msg_sys, ...)

2) In both copy_msghdr_from_user() and get_compat_msghdr(), cap
   the msg_namelen to sizeof(struct sockaddr_storage).

That should eliminate any and all problems in this area.

Thanks Dan.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch] net: heap overflow in __audit_sockaddr()
  2013-03-19 13:55 ` David Miller
@ 2013-10-02 18:58   ` Dan Carpenter
  2013-10-02 21:11     ` Ben Hutchings
  0 siblings, 1 reply; 16+ messages in thread
From: Dan Carpenter @ 2013-10-02 18:58 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, security, Jüri Aedla

We need to cap ->msg_namelen or it leads to a buffer overflow when we
to the memcpy() in __audit_sockaddr().  It requires CAP_AUDIT_CONTROL to
exploit this bug.

The call tree is:
___sys_recvmsg()
  move_addr_to_user()
    audit_sockaddr()
      __audit_sockaddr()

Reported-by: Jüri Aedla <juri.aedla@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/net/socket.c b/net/socket.c
index ebed4b6..c226ace 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1964,6 +1964,16 @@ struct used_address {
 	unsigned int name_len;
 };
 
+static int copy_msghdr_from_user(struct msghdr *kmsg,
+				 struct msghdr __user *umsg)
+{
+	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
+		return -EFAULT;
+	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
+		return -EINVAL;
+	return 0;
+}
+
 static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
 			 struct msghdr *msg_sys, unsigned int flags,
 			 struct used_address *used_address)
@@ -1982,8 +1992,11 @@ static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
 	if (MSG_CMSG_COMPAT & flags) {
 		if (get_compat_msghdr(msg_sys, msg_compat))
 			return -EFAULT;
-	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
-		return -EFAULT;
+	} else {
+		err = copy_msghdr_from_user(msg_sys, msg);
+		if (err)
+			return err;
+	}
 
 	if (msg_sys->msg_iovlen > UIO_FASTIOV) {
 		err = -EMSGSIZE;
@@ -2191,8 +2204,11 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 	if (MSG_CMSG_COMPAT & flags) {
 		if (get_compat_msghdr(msg_sys, msg_compat))
 			return -EFAULT;
-	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
-		return -EFAULT;
+	} else {
+		err = copy_msghdr_from_user(msg_sys, msg);
+		if (err)
+			return err;
+	}
 
 	if (msg_sys->msg_iovlen > UIO_FASTIOV) {
 		err = -EMSGSIZE;

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [patch] net: heap overflow in __audit_sockaddr()
  2013-10-02 18:58   ` [patch] net: heap overflow in __audit_sockaddr() Dan Carpenter
@ 2013-10-02 21:11     ` Ben Hutchings
  2013-10-02 21:26       ` Dan Carpenter
  2013-10-02 21:27       ` [patch v2] " Dan Carpenter
  0 siblings, 2 replies; 16+ messages in thread
From: Ben Hutchings @ 2013-10-02 21:11 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: David S. Miller, netdev, security, Jüri Aedla

On Wed, 2013-10-02 at 21:58 +0300, Dan Carpenter wrote:
> We need to cap ->msg_namelen or it leads to a buffer overflow when we
> to the memcpy() in __audit_sockaddr().  It requires CAP_AUDIT_CONTROL to
> exploit this bug.
> 
> The call tree is:
> ___sys_recvmsg()
>   move_addr_to_user()
>     audit_sockaddr()
>       __audit_sockaddr()
> 
> Reported-by: Jüri Aedla <juri.aedla@gmail.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/net/socket.c b/net/socket.c
> index ebed4b6..c226ace 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1964,6 +1964,16 @@ struct used_address {
>  	unsigned int name_len;
>  };
>  
> +static int copy_msghdr_from_user(struct msghdr *kmsg,
> +				 struct msghdr __user *umsg)
> +{
> +	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> +		return -EFAULT;
> +	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
>  			 struct msghdr *msg_sys, unsigned int flags,
>  			 struct used_address *used_address)
> @@ -1982,8 +1992,11 @@ static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
>  	if (MSG_CMSG_COMPAT & flags) {
>  		if (get_compat_msghdr(msg_sys, msg_compat))
>  			return -EFAULT;
> -	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
> -		return -EFAULT;
> +	} else {
> +		err = copy_msghdr_from_user(msg_sys, msg);
> +		if (err)
> +			return err;
> +	}
[...]

This doesn't cover compat tasks, since get_compat_msghdr() has no such
check.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch] net: heap overflow in __audit_sockaddr()
  2013-10-02 21:11     ` Ben Hutchings
@ 2013-10-02 21:26       ` Dan Carpenter
  2013-10-02 21:27       ` [patch v2] " Dan Carpenter
  1 sibling, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2013-10-02 21:26 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: David S. Miller, netdev, security, Jüri Aedla

On Wed, Oct 02, 2013 at 10:11:46PM +0100, Ben Hutchings wrote:
> On Wed, 2013-10-02 at 21:58 +0300, Dan Carpenter wrote:
> > We need to cap ->msg_namelen or it leads to a buffer overflow when we
> > to the memcpy() in __audit_sockaddr().  It requires CAP_AUDIT_CONTROL to
> > exploit this bug.
> > 
> > The call tree is:
> > ___sys_recvmsg()
> >   move_addr_to_user()
> >     audit_sockaddr()
> >       __audit_sockaddr()
> > 
> > Reported-by: Jüri Aedla <juri.aedla@gmail.com>
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index ebed4b6..c226ace 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1964,6 +1964,16 @@ struct used_address {
> >  	unsigned int name_len;
> >  };
> >  
> > +static int copy_msghdr_from_user(struct msghdr *kmsg,
> > +				 struct msghdr __user *umsg)
> > +{
> > +	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> > +		return -EFAULT;
> > +	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> > +		return -EINVAL;
> > +	return 0;
> > +}
> > +
> >  static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
> >  			 struct msghdr *msg_sys, unsigned int flags,
> >  			 struct used_address *used_address)
> > @@ -1982,8 +1992,11 @@ static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
> >  	if (MSG_CMSG_COMPAT & flags) {
> >  		if (get_compat_msghdr(msg_sys, msg_compat))
> >  			return -EFAULT;
> > -	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
> > -		return -EFAULT;
> > +	} else {
> > +		err = copy_msghdr_from_user(msg_sys, msg);
> > +		if (err)
> > +			return err;
> > +	}
> [...]
> 
> This doesn't cover compat tasks, since get_compat_msghdr() has no such
> check.
> 

Oops.  Gar...  Thanks for catching that.  I forgot to add that chunk to
the commit.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch v2] net: heap overflow in __audit_sockaddr()
  2013-10-02 21:11     ` Ben Hutchings
  2013-10-02 21:26       ` Dan Carpenter
@ 2013-10-02 21:27       ` Dan Carpenter
  2013-10-03 20:06         ` David Miller
  2013-11-27 11:32         ` Eric Wong
  1 sibling, 2 replies; 16+ messages in thread
From: Dan Carpenter @ 2013-10-02 21:27 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, security, Jüri Aedla

We need to cap ->msg_namelen or it leads to a buffer overflow when we
to the memcpy() in __audit_sockaddr().  It requires CAP_AUDIT_CONTROL to
exploit this bug.

The call tree is:
___sys_recvmsg()
  move_addr_to_user()
    audit_sockaddr()
      __audit_sockaddr()

Reported-by: Jüri Aedla <juri.aedla@gmail.com>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2: The limit check to the compat code was missing as pointed out by
Ben Hutchings.

diff --git a/net/socket.c b/net/socket.c
index ebed4b6..c226ace 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1964,6 +1964,16 @@ struct used_address {
 	unsigned int name_len;
 };
 
+static int copy_msghdr_from_user(struct msghdr *kmsg,
+				 struct msghdr __user *umsg)
+{
+	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
+		return -EFAULT;
+	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
+		return -EINVAL;
+	return 0;
+}
+
 static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
 			 struct msghdr *msg_sys, unsigned int flags,
 			 struct used_address *used_address)
@@ -1982,8 +1992,11 @@ static int ___sys_sendmsg(struct socket *sock, struct msghdr __user *msg,
 	if (MSG_CMSG_COMPAT & flags) {
 		if (get_compat_msghdr(msg_sys, msg_compat))
 			return -EFAULT;
-	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
-		return -EFAULT;
+	} else {
+		err = copy_msghdr_from_user(msg_sys, msg);
+		if (err)
+			return err;
+	}
 
 	if (msg_sys->msg_iovlen > UIO_FASTIOV) {
 		err = -EMSGSIZE;
@@ -2191,8 +2204,11 @@ static int ___sys_recvmsg(struct socket *sock, struct msghdr __user *msg,
 	if (MSG_CMSG_COMPAT & flags) {
 		if (get_compat_msghdr(msg_sys, msg_compat))
 			return -EFAULT;
-	} else if (copy_from_user(msg_sys, msg, sizeof(struct msghdr)))
-		return -EFAULT;
+	} else {
+		err = copy_msghdr_from_user(msg_sys, msg);
+		if (err)
+			return err;
+	}
 
 	if (msg_sys->msg_iovlen > UIO_FASTIOV) {
 		err = -EMSGSIZE;
diff --git a/net/compat.c b/net/compat.c
index f0a1ba6..8903258 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -71,6 +71,8 @@ int get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg)
 	    __get_user(kmsg->msg_controllen, &umsg->msg_controllen) ||
 	    __get_user(kmsg->msg_flags, &umsg->msg_flags))
 		return -EFAULT;
+	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
+		return -EINVAL;
 	kmsg->msg_name = compat_ptr(tmp1);
 	kmsg->msg_iov = compat_ptr(tmp2);
 	kmsg->msg_control = compat_ptr(tmp3);

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [patch v2] net: heap overflow in __audit_sockaddr()
  2013-10-02 21:27       ` [patch v2] " Dan Carpenter
@ 2013-10-03 20:06         ` David Miller
  2013-11-27 11:32         ` Eric Wong
  1 sibling, 0 replies; 16+ messages in thread
From: David Miller @ 2013-10-03 20:06 UTC (permalink / raw)
  To: dan.carpenter; +Cc: netdev, security, juri.aedla

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 3 Oct 2013 00:27:20 +0300

> We need to cap ->msg_namelen or it leads to a buffer overflow when we
> to the memcpy() in __audit_sockaddr().  It requires CAP_AUDIT_CONTROL to
> exploit this bug.
> 
> The call tree is:
> ___sys_recvmsg()
>   move_addr_to_user()
>     audit_sockaddr()
>       __audit_sockaddr()
> 
> Reported-by: Jüri Aedla <juri.aedla@gmail.com>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2: The limit check to the compat code was missing as pointed out by
> Ben Hutchings.

Applied and queued up for -stable, thanks Dan.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch v2] net: heap overflow in __audit_sockaddr()
  2013-10-02 21:27       ` [patch v2] " Dan Carpenter
  2013-10-03 20:06         ` David Miller
@ 2013-11-27 11:32         ` Eric Wong
  2013-11-27 11:51           ` Hannes Frederic Sowa
  2013-11-27 13:56           ` David Laight
  1 sibling, 2 replies; 16+ messages in thread
From: Eric Wong @ 2013-11-27 11:32 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: David S. Miller, netdev, security, Jüri Aedla, stable

Dan Carpenter <dan.carpenter@oracle.com> wrote:
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -1964,6 +1964,16 @@ struct used_address {
>  	unsigned int name_len;
>  };
>  
> +static int copy_msghdr_from_user(struct msghdr *kmsg,
> +				 struct msghdr __user *umsg)
> +{
> +	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> +		return -EFAULT;
> +	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> +		return -EINVAL;
> +	return 0;

Crap, this seems to break Ruby trunk :x
https://bugs.ruby-lang.org/issues/9124

I'm inclined to think Ruby is wrong to use a gigantic buffer, but this
may also break some other existing userspace code.  I'm not sure what
the best option since breaking userspace (even buggy userspace?) is not
taken lightly.

Is there a different way to fix this in the kernel?

Note: this doesn't affect a stable release of Ruby, yet.

(Not a Ruby developer myself, just occasional contributor).

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch v2] net: heap overflow in __audit_sockaddr()
  2013-11-27 11:32         ` Eric Wong
@ 2013-11-27 11:51           ` Hannes Frederic Sowa
  2013-11-27 12:40             ` [patch] net: clamp ->msg_namelen instead of returning an error Dan Carpenter
  2013-11-27 20:24             ` [patch v2] net: heap overflow in __audit_sockaddr() Linus Torvalds
  2013-11-27 13:56           ` David Laight
  1 sibling, 2 replies; 16+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-27 11:51 UTC (permalink / raw)
  To: Eric Wong
  Cc: Dan Carpenter, David S. Miller, netdev, security, Jüri Aedla,
	stable

On Wed, Nov 27, 2013 at 11:32:18AM +0000, Eric Wong wrote:
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1964,6 +1964,16 @@ struct used_address {
> >  	unsigned int name_len;
> >  };
> >  
> > +static int copy_msghdr_from_user(struct msghdr *kmsg,
> > +				 struct msghdr __user *umsg)
> > +{
> > +	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> > +		return -EFAULT;
> > +	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> > +		return -EINVAL;
> > +	return 0;
> 
> Crap, this seems to break Ruby trunk :x
> https://bugs.ruby-lang.org/issues/9124
> 
> I'm inclined to think Ruby is wrong to use a gigantic buffer, but this
> may also break some other existing userspace code.  I'm not sure what
> the best option since breaking userspace (even buggy userspace?) is not
> taken lightly.
> 
> Is there a different way to fix this in the kernel?
> 
> Note: this doesn't affect a stable release of Ruby, yet.

We have to clamp msg_namelen to max sizeof(struct sockaddr_storage).
The sendmsg handler will check msg_namelen again and error out correctly if
the size of msg_name is too short.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [patch] net: clamp ->msg_namelen instead of returning an error
  2013-11-27 11:51           ` Hannes Frederic Sowa
@ 2013-11-27 12:40             ` Dan Carpenter
  2013-11-27 19:42               ` Eric Wong
  2013-11-27 21:27               ` Eric Dumazet
  2013-11-27 20:24             ` [patch v2] net: heap overflow in __audit_sockaddr() Linus Torvalds
  1 sibling, 2 replies; 16+ messages in thread
From: Dan Carpenter @ 2013-11-27 12:40 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Eric Wong, Hannes Frederic Sowa

If kmsg->msg_namelen > sizeof(struct sockaddr_storage) then in the
original code that would lead to memory corruption in the kernel if you
had audit configured.  If you didn't have audit configured it was
harmless.

There are some programs such as beta versions of Ruby which use too
large of a buffer and returning an error code breaks them.  We should
clamp the ->msg_namelen value instead.

Reported-by: Eric Wong <normalperson@yhbt.net>
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/net/socket.c b/net/socket.c
index 0b18693f2be6..e83c416708af 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1973,7 +1973,7 @@ static int copy_msghdr_from_user(struct msghdr *kmsg,
 	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
 		return -EFAULT;
 	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
-		return -EINVAL;
+		kmsg->msg_namelen = sizeof(struct sockaddr_storage);
 	return 0;
 }
 
diff --git a/net/compat.c b/net/compat.c
index 618c6a8a911b..dd32e34c1e2c 100644
--- a/net/compat.c
+++ b/net/compat.c
@@ -72,7 +72,7 @@ int get_compat_msghdr(struct msghdr *kmsg, struct compat_msghdr __user *umsg)
 	    __get_user(kmsg->msg_flags, &umsg->msg_flags))
 		return -EFAULT;
 	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
-		return -EINVAL;
+		kmsg->msg_namelen = sizeof(struct sockaddr_storage);
 	kmsg->msg_name = compat_ptr(tmp1);
 	kmsg->msg_iov = compat_ptr(tmp2);
 	kmsg->msg_control = compat_ptr(tmp3);

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* RE: [patch v2] net: heap overflow in __audit_sockaddr()
  2013-11-27 11:32         ` Eric Wong
  2013-11-27 11:51           ` Hannes Frederic Sowa
@ 2013-11-27 13:56           ` David Laight
  1 sibling, 0 replies; 16+ messages in thread
From: David Laight @ 2013-11-27 13:56 UTC (permalink / raw)
  To: Eric Wong, Dan Carpenter
  Cc: David S. Miller, netdev, security, Jüri Aedla, stable

> From: Eric Wong
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > --- a/net/socket.c
> > +++ b/net/socket.c
> > @@ -1964,6 +1964,16 @@ struct used_address {
> >  	unsigned int name_len;
> >  };
> >
> > +static int copy_msghdr_from_user(struct msghdr *kmsg,
> > +				 struct msghdr __user *umsg)
> > +{
> > +	if (copy_from_user(kmsg, umsg, sizeof(struct msghdr)))
> > +		return -EFAULT;
> > +	if (kmsg->msg_namelen > sizeof(struct sockaddr_storage))
> > +		return -EINVAL;
> > +	return 0;
> 
> Crap, this seems to break Ruby trunk :x
> https://bugs.ruby-lang.org/issues/9124
> 
> I'm inclined to think Ruby is wrong to use a gigantic buffer, but this
> may also break some other existing userspace code.  I'm not sure what
> the best option since breaking userspace (even buggy userspace?) is not
> taken lightly.

Well, 'struct sockaddr_storage' is a horrid item.
I think there are people who'll say that it should never be used
to allocate an actual buffer.

If the kernel is going to write an address into the buffer, it only
needs a buffer that is long enough.
If the kernel is going to read an address, the length needs to be
appropriate for the actual protocol.

Very portable code, and code that wants to be 'future proof' against
new protocols might well use a long buffer.

	David


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch] net: clamp ->msg_namelen instead of returning an error
  2013-11-27 12:40             ` [patch] net: clamp ->msg_namelen instead of returning an error Dan Carpenter
@ 2013-11-27 19:42               ` Eric Wong
  2013-11-27 21:27               ` Eric Dumazet
  1 sibling, 0 replies; 16+ messages in thread
From: Eric Wong @ 2013-11-27 19:42 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: David S. Miller, netdev, Hannes Frederic Sowa

Dan Carpenter <dan.carpenter@oracle.com> wrote:
> If kmsg->msg_namelen > sizeof(struct sockaddr_storage) then in the
> original code that would lead to memory corruption in the kernel if you
> had audit configured.  If you didn't have audit configured it was
> harmless.
> 
> There are some programs such as beta versions of Ruby which use too
> large of a buffer and returning an error code breaks them.  We should
> clamp the ->msg_namelen value instead.
> 
> Reported-by: Eric Wong <normalperson@yhbt.net>

Thanks Dan, Ruby trunk r43886 works out-of-the-box with this fix.
Dave: please queue for stable, thanks

Tested-by: Eric Wong <normalperson@yhbt.net>

> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch v2] net: heap overflow in __audit_sockaddr()
  2013-11-27 11:51           ` Hannes Frederic Sowa
  2013-11-27 12:40             ` [patch] net: clamp ->msg_namelen instead of returning an error Dan Carpenter
@ 2013-11-27 20:24             ` Linus Torvalds
  2013-11-27 21:18               ` Hannes Frederic Sowa
  1 sibling, 1 reply; 16+ messages in thread
From: Linus Torvalds @ 2013-11-27 20:24 UTC (permalink / raw)
  To: Eric Wong, Dan Carpenter, David S. Miller, Network Development,
	security@kernel.org, Jüri Aedla, # .39.x

On Wed, Nov 27, 2013 at 3:51 AM, Hannes Frederic Sowa
<hannes@stressinduktion.org> wrote:
>
> We have to clamp msg_namelen to max sizeof(struct sockaddr_storage).
> The sendmsg handler will check msg_namelen again and error out correctly if
> the size of msg_name is too short.

Yeah, clamping sounds like the right thing to do at least for
receiving. For sending, you should say "we can't send packets that big
due to memory constraints" (of, for the case of a sockaddr, "to an
address this big"), but for receiving the size of the user space
buffer is kind of irrelevant - if the user gives a bigger buffer than
necessary, who cares? We just need to make sure that the kernel
doesn't then allocate silly-big temporary buffers internally.

There seems to be a patch floating around to clamp things already.

                Linus

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch v2] net: heap overflow in __audit_sockaddr()
  2013-11-27 20:24             ` [patch v2] net: heap overflow in __audit_sockaddr() Linus Torvalds
@ 2013-11-27 21:18               ` Hannes Frederic Sowa
  0 siblings, 0 replies; 16+ messages in thread
From: Hannes Frederic Sowa @ 2013-11-27 21:18 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: Eric Wong, Dan Carpenter, David S. Miller, Network Development,
	security@kernel.org, Jüri Aedla, # .39.x

On Wed, Nov 27, 2013 at 12:24:41PM -0800, Linus Torvalds wrote:
> On Wed, Nov 27, 2013 at 3:51 AM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
> >
> > We have to clamp msg_namelen to max sizeof(struct sockaddr_storage).
> > The sendmsg handler will check msg_namelen again and error out correctly if
> > the size of msg_name is too short.
> 
> Yeah, clamping sounds like the right thing to do at least for
> receiving. For sending, you should say "we can't send packets that big
> due to memory constraints" (of, for the case of a sockaddr, "to an
> address this big"), but for receiving the size of the user space
> buffer is kind of irrelevant - if the user gives a bigger buffer than
> necessary, who cares? We just need to make sure that the kernel
> doesn't then allocate silly-big temporary buffers internally.
>
> There seems to be a patch floating around to clamp things already.

Data buffers are clamed fine already.

The sockaddr buffers are currently always 128 bytes (== sizeof(struct
sockaddr_storage)) in size and are allocated on the stack of the
recvmsg/sendmsg syscall handlers. We normally don't have high stack usage
on recvmsg calls but it could be worth trying to optimize that on sendmsg,
I agree. I have not seen a patch which tries to do so but maybe I haven't
looked far enough back in the mailing list archives.

Clamping on sending seems necessary to not break exisiting applications. I
guess those programs expect the kernel to know which namelen the
protocol expects and only use that part of the provided sockaddr
buffer (an example is the mentioned ruby bug in this thread which
seems to pass down the size of a union for all possile sockaddrs:
<https://bugs.ruby-lang.org/issues/9124#note-2>).

I recently noticed a some more subtile annoyance in that code:

We don't know the anticipated sockaddr size before calling the recvmsg
handler. Hence it is currently possible that we dequeue a packet from
the socket receiving queue and later error out and drop the packet because
the user provided a socket address buffer which is too small. IMHO we
should catch that before dequeueing the packet. Either we can export the
address size via the per-protocol-structures or we have to start passing the
user provided buffer sizes down the stack (currently all recvmsg handlers
expect to always have 128 bytes for storing addresses).

I'll look into this.

Greetings,

  Hannes

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch] net: clamp ->msg_namelen instead of returning an error
  2013-11-27 12:40             ` [patch] net: clamp ->msg_namelen instead of returning an error Dan Carpenter
  2013-11-27 19:42               ` Eric Wong
@ 2013-11-27 21:27               ` Eric Dumazet
  2013-11-29 21:13                 ` David Miller
  1 sibling, 1 reply; 16+ messages in thread
From: Eric Dumazet @ 2013-11-27 21:27 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: David S. Miller, netdev, Eric Wong, Hannes Frederic Sowa

On Wed, 2013-11-27 at 15:40 +0300, Dan Carpenter wrote:
> If kmsg->msg_namelen > sizeof(struct sockaddr_storage) then in the
> original code that would lead to memory corruption in the kernel if you
> had audit configured.  If you didn't have audit configured it was
> harmless.
> 
> There are some programs such as beta versions of Ruby which use too
> large of a buffer and returning an error code breaks them.  We should
> clamp the ->msg_namelen value instead.
> 
> Reported-by: Eric Wong <normalperson@yhbt.net>
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Fixes: 1661bf364ae9 ("net: heap overflow in __audit_sockaddr()")

Acked-by: Eric Dumazet <edumazet@google.com>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [patch] net: clamp ->msg_namelen instead of returning an error
  2013-11-27 21:27               ` Eric Dumazet
@ 2013-11-29 21:13                 ` David Miller
  0 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2013-11-29 21:13 UTC (permalink / raw)
  To: eric.dumazet; +Cc: dan.carpenter, netdev, normalperson, hannes

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 27 Nov 2013 13:27:46 -0800

> On Wed, 2013-11-27 at 15:40 +0300, Dan Carpenter wrote:
>> If kmsg->msg_namelen > sizeof(struct sockaddr_storage) then in the
>> original code that would lead to memory corruption in the kernel if you
>> had audit configured.  If you didn't have audit configured it was
>> harmless.
>> 
>> There are some programs such as beta versions of Ruby which use too
>> large of a buffer and returning an error code breaks them.  We should
>> clamp the ->msg_namelen value instead.
>> 
>> Reported-by: Eric Wong <normalperson@yhbt.net>
>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> Fixes: 1661bf364ae9 ("net: heap overflow in __audit_sockaddr()")
> 
> Acked-by: Eric Dumazet <edumazet@google.com>

Applied and queued up for -stable, thanks everyone.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2013-11-29 21:13 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-03-18 10:10 question about klen in move_addr_to_user() Dan Carpenter
2013-03-19 13:55 ` David Miller
2013-10-02 18:58   ` [patch] net: heap overflow in __audit_sockaddr() Dan Carpenter
2013-10-02 21:11     ` Ben Hutchings
2013-10-02 21:26       ` Dan Carpenter
2013-10-02 21:27       ` [patch v2] " Dan Carpenter
2013-10-03 20:06         ` David Miller
2013-11-27 11:32         ` Eric Wong
2013-11-27 11:51           ` Hannes Frederic Sowa
2013-11-27 12:40             ` [patch] net: clamp ->msg_namelen instead of returning an error Dan Carpenter
2013-11-27 19:42               ` Eric Wong
2013-11-27 21:27               ` Eric Dumazet
2013-11-29 21:13                 ` David Miller
2013-11-27 20:24             ` [patch v2] net: heap overflow in __audit_sockaddr() Linus Torvalds
2013-11-27 21:18               ` Hannes Frederic Sowa
2013-11-27 13:56           ` David Laight

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).