Patchwork net: Check the argument for listen(2)

login
register
mail settings
Submitter Changli Gao
Date June 28, 2013, 4:14 p.m.
Message ID <1372436076-61436-1-git-send-email-xiaosuo@gmail.com>
Download mbox | patch
Permalink /patch/255460/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Changli Gao - June 28, 2013, 4:14 p.m.
As we use u16 to save the value of the argument for listen(2),
we'd better check if the value is larger than SINT_MAX other
than cut it down silently on error.
---
 net/ipv4/af_inet.c |    3 +++
 1 file changed, 3 insertions(+)
Eric Dumazet - June 28, 2013, 4:29 p.m.
On Sat, 2013-06-29 at 00:14 +0800, Changli Gao wrote:
> As we use u16 to save the value of the argument for listen(2),
> we'd better check if the value is larger than SINT_MAX other
> than cut it down silently on error.
> ---
>  net/ipv4/af_inet.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index b4d0be2..35aaf00 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -198,6 +198,9 @@ int inet_listen(struct socket *sock, int backlog)
>  	unsigned char old_state;
>  	int err;
>  
> +	if (backlog >= (1 << 16))
> +		return EINVAL;
> +
>  	lock_sock(sk);
>  
>  	err = -EINVAL;

This seems pretty buggy to me.

1) EINVAL is not -EINVAL

2) This might break existing applications.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger - June 28, 2013, 4:30 p.m.
On Sat, 29 Jun 2013 00:14:36 +0800
Changli Gao <xiaosuo@gmail.com> wrote:

> As we use u16 to save the value of the argument for listen(2),
> we'd better check if the value is larger than SINT_MAX other
> than cut it down silently on error.
> ---
>  net/ipv4/af_inet.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index b4d0be2..35aaf00 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -198,6 +198,9 @@ int inet_listen(struct socket *sock, int backlog)
>  	unsigned char old_state;
>  	int err;
>  
> +	if (backlog >= (1 << 16))
> +		return EINVAL;
> +
>  	lock_sock(sk);
>  
>  	err = -EINVAL;

Is this just a theoretical problem or is there an example?
Since this is a user API, you should check related standards, it maybe
that the kernel should just silently truncate. Returning errors
to legacy applications makes people really angry.

If you decide that returning an error is the correct behaviour
then the return value should be negative in order to follow kernel practice
and other parts of same code.

Rather than open coding >= (1<<16) might be better to > USHRT_MAX.

Lastly, a test like this deserves a comment like:
       /* since sk_max_ack_backlog is ushort */

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - June 28, 2013, 4:32 p.m.
On Fri, Jun 28, 2013 at 09:29:02AM -0700, Eric Dumazet wrote:
> On Sat, 2013-06-29 at 00:14 +0800, Changli Gao wrote:
> > --- a/net/ipv4/af_inet.c
> > +++ b/net/ipv4/af_inet.c
> > @@ -198,6 +198,9 @@ int inet_listen(struct socket *sock, int backlog)
> >  	unsigned char old_state;
> >  	int err;
> >  
> > +	if (backlog >= (1 << 16))
> > +		return EINVAL;
> > +
> >  	lock_sock(sk);
> >  
> >  	err = -EINVAL;
> 
> This seems pretty buggy to me.
> 
> 1) EINVAL is not -EINVAL
> 
> 2) This might break existing applications.

Yes, we should clamp the value to netdev_max_backlog.

Greetings,

  Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa - June 28, 2013, 4:34 p.m.
On Fri, Jun 28, 2013 at 06:32:34PM +0200, Hannes Frederic Sowa wrote:
> On Fri, Jun 28, 2013 at 09:29:02AM -0700, Eric Dumazet wrote:
> > On Sat, 2013-06-29 at 00:14 +0800, Changli Gao wrote:
> > > --- a/net/ipv4/af_inet.c
> > > +++ b/net/ipv4/af_inet.c
> > > @@ -198,6 +198,9 @@ int inet_listen(struct socket *sock, int backlog)
> > >  	unsigned char old_state;
> > >  	int err;
> > >  
> > > +	if (backlog >= (1 << 16))
> > > +		return EINVAL;
> > > +
> > >  	lock_sock(sk);
> > >  
> > >  	err = -EINVAL;
> > 
> > This seems pretty buggy to me.
> > 
> > 1) EINVAL is not -EINVAL
> > 
> > 2) This might break existing applications.
> 
> Yes, we should clamp the value to netdev_max_backlog.

Nonsense, somaxconn would make more sense.

Sorry,

  Hannes
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet - June 28, 2013, 4:48 p.m.
On Fri, 2013-06-28 at 18:34 +0200, Hannes Frederic Sowa wrote:

> Nonsense, somaxconn would make more sense.
> 


If we keep u16, we could limit the sysctl (somaxconn,
tcp_max_syn_backlog) limits to 65535



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index b4d0be2..35aaf00 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -198,6 +198,9 @@  int inet_listen(struct socket *sock, int backlog)
 	unsigned char old_state;
 	int err;
 
+	if (backlog >= (1 << 16))
+		return EINVAL;
+
 	lock_sock(sk);
 
 	err = -EINVAL;