[PATCH net-next 0/4] sctp: clean up __sctp_connect function

[PATCH net-next 0/4] sctp: clean up __sctp_connect function

[Xin Long]

This patchset is to factor out some common code for
sctp_sendmsg_new_asoc() and __sctp_connect() into 2
new functioins.

Xin Long (4):
  sctp: check addr_size with sa_family_t size in
    __sctp_setsockopt_connectx
  sctp: clean up __sctp_connect
  sctp: factor out sctp_connect_new_asoc
  sctp: factor out sctp_connect_add_peer

 net/sctp/socket.c | 377 +++++++++++++++++++++---------------------------------
 1 file changed, 147 insertions(+), 230 deletions(-)

-- 
2.1.0

[PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Xin Long]

Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
sctp_inet_connect(), the latter has done addr_size check with size
of sa_family_t.

In the next patch to clean up __sctp_connect(), we will remove
addr_size check with size of sa_family_t from __sctp_connect()
for the 1st address.

So before doing that, __sctp_setsockopt_connectx() should do
this check first, as sctp_inet_connect() does.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index aa80cda..5f92e4a 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
 	pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
 		 __func__, sk, addrs, addrs_size);
 
-	if (unlikely(addrs_size <= 0))
+	if (unlikely(addrs_size < sizeof(sa_family_t)))
 		return -EINVAL;
 
 	kaddrs = memdup_user(addrs, addrs_size);
-- 
2.1.0

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Neil Horman]

On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> sctp_inet_connect(), the latter has done addr_size check with size
> of sa_family_t.
> 
> In the next patch to clean up __sctp_connect(), we will remove
> addr_size check with size of sa_family_t from __sctp_connect()
> for the 1st address.
> 
> So before doing that, __sctp_setsockopt_connectx() should do
> this check first, as sctp_inet_connect() does.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index aa80cda..5f92e4a 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
>  	pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
>  		 __func__, sk, addrs, addrs_size);
>  
> -	if (unlikely(addrs_size <= 0))
> +	if (unlikely(addrs_size < sizeof(sa_family_t)))
I don't think this is what you want to check for here.  sa_family_t is
an unsigned short, and addrs_size is the number of bytes in the addrs
array.  The addrs array should be at least the size of one struct
sockaddr (16 bytes iirc), and, if larger, should be a multiple of
sizeof(struct sockaddr)

Neil

>  		return -EINVAL;
>  
>  	kaddrs = memdup_user(addrs, addrs_size);
> -- 
> 2.1.0
> 
> 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Xin Long]

On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > sctp_inet_connect(), the latter has done addr_size check with size
> > of sa_family_t.
> >
> > In the next patch to clean up __sctp_connect(), we will remove
> > addr_size check with size of sa_family_t from __sctp_connect()
> > for the 1st address.
> >
> > So before doing that, __sctp_setsockopt_connectx() should do
> > this check first, as sctp_inet_connect() does.
> >
> > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > ---
> >  net/sctp/socket.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > index aa80cda..5f92e4a 100644
> > --- a/net/sctp/socket.c
> > +++ b/net/sctp/socket.c
> > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> >                __func__, sk, addrs, addrs_size);
> >
> > -     if (unlikely(addrs_size <= 0))
> > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> I don't think this is what you want to check for here.  sa_family_t is
> an unsigned short, and addrs_size is the number of bytes in the addrs
> array.  The addrs array should be at least the size of one struct
> sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> sizeof(struct sockaddr)
sizeof(struct sockaddr) is not the right value to check either.

The proper check will be done later in __sctp_connect():

        af = sctp_get_af_specific(daddr->sa.sa_family);
        if (!af || af->sockaddr_len > addrs_size)
                return -EINVAL;

So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
just to make sure daddr->sa.sa_family is accessible. the same
check is also done in sctp_inet_connect().

>
> Neil
>
> >               return -EINVAL;
> >
> >       kaddrs = memdup_user(addrs, addrs_size);
> > --
> > 2.1.0
> >
> >

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Neil Horman]

On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > sctp_inet_connect(), the latter has done addr_size check with size
> > > of sa_family_t.
> > >
> > > In the next patch to clean up __sctp_connect(), we will remove
> > > addr_size check with size of sa_family_t from __sctp_connect()
> > > for the 1st address.
> > >
> > > So before doing that, __sctp_setsockopt_connectx() should do
> > > this check first, as sctp_inet_connect() does.
> > >
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > >  net/sctp/socket.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > index aa80cda..5f92e4a 100644
> > > --- a/net/sctp/socket.c
> > > +++ b/net/sctp/socket.c
> > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > >                __func__, sk, addrs, addrs_size);
> > >
> > > -     if (unlikely(addrs_size <= 0))
> > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > I don't think this is what you want to check for here.  sa_family_t is
> > an unsigned short, and addrs_size is the number of bytes in the addrs
> > array.  The addrs array should be at least the size of one struct
> > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > sizeof(struct sockaddr)
> sizeof(struct sockaddr) is not the right value to check either.
> 
> The proper check will be done later in __sctp_connect():
> 
>         af = sctp_get_af_specific(daddr->sa.sa_family);
>         if (!af || af->sockaddr_len > addrs_size)
>                 return -EINVAL;
> 
> So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> just to make sure daddr->sa.sa_family is accessible. the same
> check is also done in sctp_inet_connect().
> 
That doesn't make much sense, if the proper check is done in __sctp_connect with
the size of the families sockaddr_len, then we don't need this check at all, we
can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
we get that from memdup_user, we know its not accessible, and can bail out.

About the only thing we need to check for here is that addr_len isn't some
absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
upwards of 2G in memdup_user.  Your change does that just fine, but its no
better or worse than checking for <=0

Neil

> >
> > Neil
> >
> > >               return -EINVAL;
> > >
> > >       kaddrs = memdup_user(addrs, addrs_size);
> > > --
> > > 2.1.0
> > >
> > >
> 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Marcelo Ricardo Leitner]

On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > >
> > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > of sa_family_t.
> > > >
> > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > for the 1st address.
> > > >
> > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > this check first, as sctp_inet_connect() does.
> > > >
> > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > ---
> > > >  net/sctp/socket.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > index aa80cda..5f92e4a 100644
> > > > --- a/net/sctp/socket.c
> > > > +++ b/net/sctp/socket.c
> > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > >                __func__, sk, addrs, addrs_size);
> > > >
> > > > -     if (unlikely(addrs_size <= 0))
> > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > I don't think this is what you want to check for here.  sa_family_t is
> > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > array.  The addrs array should be at least the size of one struct
> > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > sizeof(struct sockaddr)
> > sizeof(struct sockaddr) is not the right value to check either.
> > 
> > The proper check will be done later in __sctp_connect():
> > 
> >         af = sctp_get_af_specific(daddr->sa.sa_family);
> >         if (!af || af->sockaddr_len > addrs_size)
> >                 return -EINVAL;
> > 
> > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > just to make sure daddr->sa.sa_family is accessible. the same
> > check is also done in sctp_inet_connect().
> > 
> That doesn't make much sense, if the proper check is done in __sctp_connect with
> the size of the families sockaddr_len, then we don't need this check at all, we
> can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> we get that from memdup_user, we know its not accessible, and can bail out.
> 
> About the only thing we need to check for here is that addr_len isn't some
> absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> upwards of 2G in memdup_user.  Your change does that just fine, but its no
> better or worse than checking for <=0

One can argue that such check against absurdly high values is random
and not effective, as 2G can be somewhat reasonable on 8GB systems but
certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
gracefully as it uses GFP_USER and __GFP_NOWARN.

The original check is more for protecting for sane usage of the
variable, which is an int, and a negative value is questionable. We
could cast, yes, but.. was that really the intent of the application?
Probably not.

> 
> Neil
> 
> > >
> > > Neil
> > >
> > > >               return -EINVAL;
> > > >
> > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > --
> > > > 2.1.0
> > > >
> > > >
> > 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Marcelo Ricardo Leitner]

On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > >
> > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > of sa_family_t.
> > > > >
> > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > for the 1st address.
> > > > >
> > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > this check first, as sctp_inet_connect() does.
> > > > >
> > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > ---
> > > > >  net/sctp/socket.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > index aa80cda..5f92e4a 100644
> > > > > --- a/net/sctp/socket.c
> > > > > +++ b/net/sctp/socket.c
> > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > >                __func__, sk, addrs, addrs_size);
> > > > >
> > > > > -     if (unlikely(addrs_size <= 0))
> > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > array.  The addrs array should be at least the size of one struct
> > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > sizeof(struct sockaddr)
> > > sizeof(struct sockaddr) is not the right value to check either.
> > > 
> > > The proper check will be done later in __sctp_connect():
> > > 
> > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > >         if (!af || af->sockaddr_len > addrs_size)
> > >                 return -EINVAL;
> > > 
> > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > just to make sure daddr->sa.sa_family is accessible. the same
> > > check is also done in sctp_inet_connect().
> > > 
> > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > the size of the families sockaddr_len, then we don't need this check at all, we
> > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > we get that from memdup_user, we know its not accessible, and can bail out.
> > 
> > About the only thing we need to check for here is that addr_len isn't some
> > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > better or worse than checking for <=0
> 
> One can argue that such check against absurdly high values is random
> and not effective, as 2G can be somewhat reasonable on 8GB systems but
> certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> gracefully as it uses GFP_USER and __GFP_NOWARN.
> 
> The original check is more for protecting for sane usage of the
> variable, which is an int, and a negative value is questionable. We
> could cast, yes, but.. was that really the intent of the application?
> Probably not.

Though that said, I'm okay with the new check here: a quick sanity
check that can avoid expensive calls to kmalloc(), while more refined
check is done later on.

> 
> > 
> > Neil
> > 
> > > >
> > > > Neil
> > > >
> > > > >               return -EINVAL;
> > > > >
> > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > --
> > > > > 2.1.0
> > > > >
> > > > >
> > > 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Neil Horman]

On Wed, Jul 24, 2019 at 09:49:07AM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > >
> > > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > > of sa_family_t.
> > > > > >
> > > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > > for the 1st address.
> > > > > >
> > > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > > this check first, as sctp_inet_connect() does.
> > > > > >
> > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > ---
> > > > > >  net/sctp/socket.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > index aa80cda..5f92e4a 100644
> > > > > > --- a/net/sctp/socket.c
> > > > > > +++ b/net/sctp/socket.c
> > > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > > >                __func__, sk, addrs, addrs_size);
> > > > > >
> > > > > > -     if (unlikely(addrs_size <= 0))
> > > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > > array.  The addrs array should be at least the size of one struct
> > > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > > sizeof(struct sockaddr)
> > > > sizeof(struct sockaddr) is not the right value to check either.
> > > > 
> > > > The proper check will be done later in __sctp_connect():
> > > > 
> > > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > > >         if (!af || af->sockaddr_len > addrs_size)
> > > >                 return -EINVAL;
> > > > 
> > > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > > just to make sure daddr->sa.sa_family is accessible. the same
> > > > check is also done in sctp_inet_connect().
> > > > 
> > > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > > the size of the families sockaddr_len, then we don't need this check at all, we
> > > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > > we get that from memdup_user, we know its not accessible, and can bail out.
> > > 
> > > About the only thing we need to check for here is that addr_len isn't some
> > > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > > better or worse than checking for <=0
> > 
> > One can argue that such check against absurdly high values is random
> > and not effective, as 2G can be somewhat reasonable on 8GB systems but
> > certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> > gracefully as it uses GFP_USER and __GFP_NOWARN.
> > 
> > The original check is more for protecting for sane usage of the
> > variable, which is an int, and a negative value is questionable. We
> > could cast, yes, but.. was that really the intent of the application?
> > Probably not.
> 
> Though that said, I'm okay with the new check here: a quick sanity
> check that can avoid expensive calls to kmalloc(), while more refined
> check is done later on.
> 
I agree a sanity check makes sense, just to avoid allocating a huge value
(even 2G is absurd on many systems), however, I'm not super comfortable with
checking for the value being less than 16 (sizeof(sa_family_t)).  The zero check
is fairly obvious given the signed nature of the lengh field, this check makes
me wonder what exactly we are checking for.

Neil

> > 
> > > 
> > > Neil
> > > 
> > > > >
> > > > > Neil
> > > > >
> > > > > >               return -EINVAL;
> > > > > >
> > > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > > --
> > > > > > 2.1.0
> > > > > >
> > > > > >
> > > > 
> 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Marcelo Ricardo Leitner]

On Wed, Jul 24, 2019 at 02:44:56PM -0400, Neil Horman wrote:
> On Wed, Jul 24, 2019 at 09:49:07AM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > > > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > >
> > > > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > > > of sa_family_t.
> > > > > > >
> > > > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > > > for the 1st address.
> > > > > > >
> > > > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > > > this check first, as sctp_inet_connect() does.
> > > > > > >
> > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > ---
> > > > > > >  net/sctp/socket.c | 2 +-
> > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > > index aa80cda..5f92e4a 100644
> > > > > > > --- a/net/sctp/socket.c
> > > > > > > +++ b/net/sctp/socket.c
> > > > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > > > >                __func__, sk, addrs, addrs_size);
> > > > > > >
> > > > > > > -     if (unlikely(addrs_size <= 0))
> > > > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > > > array.  The addrs array should be at least the size of one struct
> > > > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > > > sizeof(struct sockaddr)
> > > > > sizeof(struct sockaddr) is not the right value to check either.
> > > > > 
> > > > > The proper check will be done later in __sctp_connect():
> > > > > 
> > > > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > > > >         if (!af || af->sockaddr_len > addrs_size)
> > > > >                 return -EINVAL;
> > > > > 
> > > > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > > > just to make sure daddr->sa.sa_family is accessible. the same
> > > > > check is also done in sctp_inet_connect().
> > > > > 
> > > > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > > > the size of the families sockaddr_len, then we don't need this check at all, we
> > > > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > > > we get that from memdup_user, we know its not accessible, and can bail out.
> > > > 
> > > > About the only thing we need to check for here is that addr_len isn't some
> > > > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > > > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > > > better or worse than checking for <=0
> > > 
> > > One can argue that such check against absurdly high values is random
> > > and not effective, as 2G can be somewhat reasonable on 8GB systems but
> > > certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> > > gracefully as it uses GFP_USER and __GFP_NOWARN.
> > > 
> > > The original check is more for protecting for sane usage of the
> > > variable, which is an int, and a negative value is questionable. We
> > > could cast, yes, but.. was that really the intent of the application?
> > > Probably not.
> > 
> > Though that said, I'm okay with the new check here: a quick sanity
> > check that can avoid expensive calls to kmalloc(), while more refined
> > check is done later on.
> > 
> I agree a sanity check makes sense, just to avoid allocating a huge value
> (even 2G is absurd on many systems), however, I'm not super comfortable with
> checking for the value being less than 16 (sizeof(sa_family_t)).  The zero check

16 bits you mean then, per
include/uapi/linux/socket.h
typedef unsigned short __kernel_sa_family_t;
include/linux/socket.h
typedef __kernel_sa_family_t    sa_family_t;

> is fairly obvious given the signed nature of the lengh field, this check makes
> me wonder what exactly we are checking for.

A minimum viable buffer without doing more extensive tests. Beyond
sa_family, we need to parse sa_family and then that's left for later.
Perhaps a comment helps, something like
	/* Check if we have at least the family type in there */
?

  Marcelo

> 
> Neil
> 
> > > 
> > > > 
> > > > Neil
> > > > 
> > > > > >
> > > > > > Neil
> > > > > >
> > > > > > >               return -EINVAL;
> > > > > > >
> > > > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > > > --
> > > > > > > 2.1.0
> > > > > > >
> > > > > > >
> > > > > 
> > 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Marcelo Ricardo Leitner]

On Wed, Jul 24, 2019 at 04:05:43PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 24, 2019 at 02:44:56PM -0400, Neil Horman wrote:
> > On Wed, Jul 24, 2019 at 09:49:07AM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> > > > On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > > > > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > > > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > > > > of sa_family_t.
> > > > > > > >
> > > > > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > > > > for the 1st address.
> > > > > > > >
> > > > > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > > > > this check first, as sctp_inet_connect() does.
> > > > > > > >
> > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > > ---
> > > > > > > >  net/sctp/socket.c | 2 +-
> > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > > > index aa80cda..5f92e4a 100644
> > > > > > > > --- a/net/sctp/socket.c
> > > > > > > > +++ b/net/sctp/socket.c
> > > > > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > > > > >                __func__, sk, addrs, addrs_size);
> > > > > > > >
> > > > > > > > -     if (unlikely(addrs_size <= 0))
> > > > > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > > > > array.  The addrs array should be at least the size of one struct
> > > > > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > > > > sizeof(struct sockaddr)
> > > > > > sizeof(struct sockaddr) is not the right value to check either.
> > > > > > 
> > > > > > The proper check will be done later in __sctp_connect():
> > > > > > 
> > > > > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > > > > >         if (!af || af->sockaddr_len > addrs_size)
> > > > > >                 return -EINVAL;
> > > > > > 
> > > > > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > > > > just to make sure daddr->sa.sa_family is accessible. the same
> > > > > > check is also done in sctp_inet_connect().
> > > > > > 
> > > > > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > > > > the size of the families sockaddr_len, then we don't need this check at all, we
> > > > > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > > > > we get that from memdup_user, we know its not accessible, and can bail out.
> > > > > 
> > > > > About the only thing we need to check for here is that addr_len isn't some
> > > > > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > > > > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > > > > better or worse than checking for <=0
> > > > 
> > > > One can argue that such check against absurdly high values is random
> > > > and not effective, as 2G can be somewhat reasonable on 8GB systems but
> > > > certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> > > > gracefully as it uses GFP_USER and __GFP_NOWARN.
> > > > 
> > > > The original check is more for protecting for sane usage of the
> > > > variable, which is an int, and a negative value is questionable. We
> > > > could cast, yes, but.. was that really the intent of the application?
> > > > Probably not.
> > > 
> > > Though that said, I'm okay with the new check here: a quick sanity
> > > check that can avoid expensive calls to kmalloc(), while more refined
> > > check is done later on.
> > > 
> > I agree a sanity check makes sense, just to avoid allocating a huge value
> > (even 2G is absurd on many systems), however, I'm not super comfortable with
> > checking for the value being less than 16 (sizeof(sa_family_t)).  The zero check
> 
> 16 bits you mean then, per
> include/uapi/linux/socket.h
> typedef unsigned short __kernel_sa_family_t;
> include/linux/socket.h
> typedef __kernel_sa_family_t    sa_family_t;
> 
> > is fairly obvious given the signed nature of the lengh field, this check makes
> > me wonder what exactly we are checking for.
> 
> A minimum viable buffer without doing more extensive tests. Beyond
> sa_family, we need to parse sa_family and then that's left for later.
> Perhaps a comment helps, something like
> 	/* Check if we have at least the family type in there */
> ?

Hm, then this could be
-     if (unlikely(addrs_size <= 0))
+     if (unlikely(addrs_size < sizeof(struct sockaddr_in)))
(ipv4)
As it can't be smaller than that, always.

> 
>   Marcelo
> 
> > 
> > Neil
> > 
> > > > 
> > > > > 
> > > > > Neil
> > > > > 
> > > > > > >
> > > > > > > Neil
> > > > > > >
> > > > > > > >               return -EINVAL;
> > > > > > > >
> > > > > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > > > > --
> > > > > > > > 2.1.0
> > > > > > > >
> > > > > > > >
> > > > > > 
> > > 
> 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Neil Horman]

On Wed, Jul 24, 2019 at 04:12:43PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 24, 2019 at 04:05:43PM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jul 24, 2019 at 02:44:56PM -0400, Neil Horman wrote:
> > > On Wed, Jul 24, 2019 at 09:49:07AM -0300, Marcelo Ricardo Leitner wrote:
> > > > On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> > > > > On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > > > > > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > > > > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > > > >
> > > > > > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > > > > > of sa_family_t.
> > > > > > > > >
> > > > > > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > > > > > for the 1st address.
> > > > > > > > >
> > > > > > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > > > > > this check first, as sctp_inet_connect() does.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > > > ---
> > > > > > > > >  net/sctp/socket.c | 2 +-
> > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > >
> > > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > > > > index aa80cda..5f92e4a 100644
> > > > > > > > > --- a/net/sctp/socket.c
> > > > > > > > > +++ b/net/sctp/socket.c
> > > > > > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > > > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > > > > > >                __func__, sk, addrs, addrs_size);
> > > > > > > > >
> > > > > > > > > -     if (unlikely(addrs_size <= 0))
> > > > > > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > > > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > > > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > > > > > array.  The addrs array should be at least the size of one struct
> > > > > > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > > > > > sizeof(struct sockaddr)
> > > > > > > sizeof(struct sockaddr) is not the right value to check either.
> > > > > > > 
> > > > > > > The proper check will be done later in __sctp_connect():
> > > > > > > 
> > > > > > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > > > > > >         if (!af || af->sockaddr_len > addrs_size)
> > > > > > >                 return -EINVAL;
> > > > > > > 
> > > > > > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > > > > > just to make sure daddr->sa.sa_family is accessible. the same
> > > > > > > check is also done in sctp_inet_connect().
> > > > > > > 
> > > > > > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > > > > > the size of the families sockaddr_len, then we don't need this check at all, we
> > > > > > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > > > > > we get that from memdup_user, we know its not accessible, and can bail out.
> > > > > > 
> > > > > > About the only thing we need to check for here is that addr_len isn't some
> > > > > > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > > > > > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > > > > > better or worse than checking for <=0
> > > > > 
> > > > > One can argue that such check against absurdly high values is random
> > > > > and not effective, as 2G can be somewhat reasonable on 8GB systems but
> > > > > certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> > > > > gracefully as it uses GFP_USER and __GFP_NOWARN.
> > > > > 
> > > > > The original check is more for protecting for sane usage of the
> > > > > variable, which is an int, and a negative value is questionable. We
> > > > > could cast, yes, but.. was that really the intent of the application?
> > > > > Probably not.
> > > > 
> > > > Though that said, I'm okay with the new check here: a quick sanity
> > > > check that can avoid expensive calls to kmalloc(), while more refined
> > > > check is done later on.
> > > > 
> > > I agree a sanity check makes sense, just to avoid allocating a huge value
> > > (even 2G is absurd on many systems), however, I'm not super comfortable with
> > > checking for the value being less than 16 (sizeof(sa_family_t)).  The zero check
> > 
> > 16 bits you mean then, per
> > include/uapi/linux/socket.h
> > typedef unsigned short __kernel_sa_family_t;
> > include/linux/socket.h
> > typedef __kernel_sa_family_t    sa_family_t;
> > 
> > > is fairly obvious given the signed nature of the lengh field, this check makes
> > > me wonder what exactly we are checking for.
> > 
> > A minimum viable buffer without doing more extensive tests. Beyond
> > sa_family, we need to parse sa_family and then that's left for later.
> > Perhaps a comment helps, something like
> > 	/* Check if we have at least the family type in there */
> > ?
> 
> Hm, then this could be
> -     if (unlikely(addrs_size <= 0))
> +     if (unlikely(addrs_size < sizeof(struct sockaddr_in)))
> (ipv4)
> As it can't be smaller than that, always.
> 
True, but I think perhaps just the family type size check is more correct, as
thats the minimal information we need to get the proper sockaddr_len out of
sctp_get_af_specific.

Neil

> > 
> >   Marcelo
> > 
> > > 
> > > Neil
> > > 
> > > > > 
> > > > > > 
> > > > > > Neil
> > > > > > 
> > > > > > > >
> > > > > > > > Neil
> > > > > > > >
> > > > > > > > >               return -EINVAL;
> > > > > > > > >
> > > > > > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > > > > > --
> > > > > > > > > 2.1.0
> > > > > > > > >
> > > > > > > > >
> > > > > > > 
> > > > 
> > 
> 

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Xin Long]

On Thu, Jul 25, 2019 at 4:44 AM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Wed, Jul 24, 2019 at 04:12:43PM -0300, Marcelo Ricardo Leitner wrote:
> > On Wed, Jul 24, 2019 at 04:05:43PM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, Jul 24, 2019 at 02:44:56PM -0400, Neil Horman wrote:
> > > > On Wed, Jul 24, 2019 at 09:49:07AM -0300, Marcelo Ricardo Leitner wrote:
> > > > > On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> > > > > > On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > > > > > > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > > > > > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > > > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > > > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > > > > > > of sa_family_t.
> > > > > > > > > >
> > > > > > > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > > > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > > > > > > for the 1st address.
> > > > > > > > > >
> > > > > > > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > > > > > > this check first, as sctp_inet_connect() does.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >  net/sctp/socket.c | 2 +-
> > > > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > > > > > index aa80cda..5f92e4a 100644
> > > > > > > > > > --- a/net/sctp/socket.c
> > > > > > > > > > +++ b/net/sctp/socket.c
> > > > > > > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > > > > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > > > > > > >                __func__, sk, addrs, addrs_size);
> > > > > > > > > >
> > > > > > > > > > -     if (unlikely(addrs_size <= 0))
> > > > > > > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > > > > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > > > > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > > > > > > array.  The addrs array should be at least the size of one struct
> > > > > > > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > > > > > > sizeof(struct sockaddr)
> > > > > > > > sizeof(struct sockaddr) is not the right value to check either.
> > > > > > > >
> > > > > > > > The proper check will be done later in __sctp_connect():
> > > > > > > >
> > > > > > > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > > > > > > >         if (!af || af->sockaddr_len > addrs_size)
> > > > > > > >                 return -EINVAL;
> > > > > > > >
> > > > > > > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > > > > > > just to make sure daddr->sa.sa_family is accessible. the same
> > > > > > > > check is also done in sctp_inet_connect().
> > > > > > > >
> > > > > > > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > > > > > > the size of the families sockaddr_len, then we don't need this check at all, we
> > > > > > > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > > > > > > we get that from memdup_user, we know its not accessible, and can bail out.
> > > > > > >
> > > > > > > About the only thing we need to check for here is that addr_len isn't some
> > > > > > > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > > > > > > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > > > > > > better or worse than checking for <=0
> > > > > >
> > > > > > One can argue that such check against absurdly high values is random
> > > > > > and not effective, as 2G can be somewhat reasonable on 8GB systems but
> > > > > > certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> > > > > > gracefully as it uses GFP_USER and __GFP_NOWARN.
> > > > > >
> > > > > > The original check is more for protecting for sane usage of the
> > > > > > variable, which is an int, and a negative value is questionable. We
> > > > > > could cast, yes, but.. was that really the intent of the application?
> > > > > > Probably not.
> > > > >
> > > > > Though that said, I'm okay with the new check here: a quick sanity
> > > > > check that can avoid expensive calls to kmalloc(), while more refined
> > > > > check is done later on.
> > > > >
> > > > I agree a sanity check makes sense, just to avoid allocating a huge value
> > > > (even 2G is absurd on many systems), however, I'm not super comfortable with
> > > > checking for the value being less than 16 (sizeof(sa_family_t)).  The zero check
> > >
> > > 16 bits you mean then, per
> > > include/uapi/linux/socket.h
> > > typedef unsigned short __kernel_sa_family_t;
> > > include/linux/socket.h
> > > typedef __kernel_sa_family_t    sa_family_t;
> > >
> > > > is fairly obvious given the signed nature of the lengh field, this check makes
> > > > me wonder what exactly we are checking for.
> > >
> > > A minimum viable buffer without doing more extensive tests. Beyond
> > > sa_family, we need to parse sa_family and then that's left for later.
> > > Perhaps a comment helps, something like
> > >     /* Check if we have at least the family type in there */
> > > ?
> >
> > Hm, then this could be
> > -     if (unlikely(addrs_size <= 0))
> > +     if (unlikely(addrs_size < sizeof(struct sockaddr_in)))
> > (ipv4)
> > As it can't be smaller than that, always.
> >
> True, but I think perhaps just the family type size check is more correct, as
> thats the minimal information we need to get the proper sockaddr_len out of
> sctp_get_af_specific.
Okay, I will keep the check "addrs_size < sizeof(sa_family_t)" in this
patch and remove the useless variables in patch 2/4 when sending v2.

Thanks.

>
> Neil
>
> > >
> > >   Marcelo
> > >
> > > >
> > > > Neil
> > > >
> > > > > >
> > > > > > >
> > > > > > > Neil
> > > > > > >
> > > > > > > > >
> > > > > > > > > Neil
> > > > > > > > >
> > > > > > > > > >               return -EINVAL;
> > > > > > > > > >
> > > > > > > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > > > > > > --
> > > > > > > > > > 2.1.0
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > >
> > > > >
> > >
> >

Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx

[Neil Horman]

On Wed, Jul 24, 2019 at 04:05:43PM -0300, Marcelo Ricardo Leitner wrote:
> On Wed, Jul 24, 2019 at 02:44:56PM -0400, Neil Horman wrote:
> > On Wed, Jul 24, 2019 at 09:49:07AM -0300, Marcelo Ricardo Leitner wrote:
> > > On Wed, Jul 24, 2019 at 09:36:50AM -0300, Marcelo Ricardo Leitner wrote:
> > > > On Wed, Jul 24, 2019 at 07:22:35AM -0400, Neil Horman wrote:
> > > > > On Wed, Jul 24, 2019 at 03:21:12PM +0800, Xin Long wrote:
> > > > > > On Tue, Jul 23, 2019 at 11:25 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > > > > >
> > > > > > > On Tue, Jul 23, 2019 at 01:37:57AM +0800, Xin Long wrote:
> > > > > > > > Now __sctp_connect() is called by __sctp_setsockopt_connectx() and
> > > > > > > > sctp_inet_connect(), the latter has done addr_size check with size
> > > > > > > > of sa_family_t.
> > > > > > > >
> > > > > > > > In the next patch to clean up __sctp_connect(), we will remove
> > > > > > > > addr_size check with size of sa_family_t from __sctp_connect()
> > > > > > > > for the 1st address.
> > > > > > > >
> > > > > > > > So before doing that, __sctp_setsockopt_connectx() should do
> > > > > > > > this check first, as sctp_inet_connect() does.
> > > > > > > >
> > > > > > > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > > > > > > ---
> > > > > > > >  net/sctp/socket.c | 2 +-
> > > > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> > > > > > > > index aa80cda..5f92e4a 100644
> > > > > > > > --- a/net/sctp/socket.c
> > > > > > > > +++ b/net/sctp/socket.c
> > > > > > > > @@ -1311,7 +1311,7 @@ static int __sctp_setsockopt_connectx(struct sock *sk,
> > > > > > > >       pr_debug("%s: sk:%p addrs:%p addrs_size:%d\n",
> > > > > > > >                __func__, sk, addrs, addrs_size);
> > > > > > > >
> > > > > > > > -     if (unlikely(addrs_size <= 0))
> > > > > > > > +     if (unlikely(addrs_size < sizeof(sa_family_t)))
> > > > > > > I don't think this is what you want to check for here.  sa_family_t is
> > > > > > > an unsigned short, and addrs_size is the number of bytes in the addrs
> > > > > > > array.  The addrs array should be at least the size of one struct
> > > > > > > sockaddr (16 bytes iirc), and, if larger, should be a multiple of
> > > > > > > sizeof(struct sockaddr)
> > > > > > sizeof(struct sockaddr) is not the right value to check either.
> > > > > > 
> > > > > > The proper check will be done later in __sctp_connect():
> > > > > > 
> > > > > >         af = sctp_get_af_specific(daddr->sa.sa_family);
> > > > > >         if (!af || af->sockaddr_len > addrs_size)
> > > > > >                 return -EINVAL;
> > > > > > 
> > > > > > So the check 'addrs_size < sizeof(sa_family_t)' in this patch is
> > > > > > just to make sure daddr->sa.sa_family is accessible. the same
> > > > > > check is also done in sctp_inet_connect().
> > > > > > 
> > > > > That doesn't make much sense, if the proper check is done in __sctp_connect with
> > > > > the size of the families sockaddr_len, then we don't need this check at all, we
> > > > > can just let memdup_user take the fault on copy_to_user and return -EFAULT.  If
> > > > > we get that from memdup_user, we know its not accessible, and can bail out.
> > > > > 
> > > > > About the only thing we need to check for here is that addr_len isn't some
> > > > > absurdly high value (i.e. a negative value), so that we avoid trying to kmalloc
> > > > > upwards of 2G in memdup_user.  Your change does that just fine, but its no
> > > > > better or worse than checking for <=0
> > > > 
> > > > One can argue that such check against absurdly high values is random
> > > > and not effective, as 2G can be somewhat reasonable on 8GB systems but
> > > > certainly isn't on 512MB ones. On that, kmemdup_user() will also fail
> > > > gracefully as it uses GFP_USER and __GFP_NOWARN.
> > > > 
> > > > The original check is more for protecting for sane usage of the
> > > > variable, which is an int, and a negative value is questionable. We
> > > > could cast, yes, but.. was that really the intent of the application?
> > > > Probably not.
> > > 
> > > Though that said, I'm okay with the new check here: a quick sanity
> > > check that can avoid expensive calls to kmalloc(), while more refined
> > > check is done later on.
> > > 
> > I agree a sanity check makes sense, just to avoid allocating a huge value
> > (even 2G is absurd on many systems), however, I'm not super comfortable with
> > checking for the value being less than 16 (sizeof(sa_family_t)).  The zero check
> 
> 16 bits you mean then, per
> include/uapi/linux/socket.h
> typedef unsigned short __kernel_sa_family_t;
> include/linux/socket.h
> typedef __kernel_sa_family_t    sa_family_t;
> 
> > is fairly obvious given the signed nature of the lengh field, this check makes
> > me wonder what exactly we are checking for.
> 
> A minimum viable buffer without doing more extensive tests. Beyond
> sa_family, we need to parse sa_family and then that's left for later.
> Perhaps a comment helps, something like
> 	/* Check if we have at least the family type in there */
> ?
> 
Yeah, I'd be ok with something like that, or perhaps:

/* Check if we have at least the family value in buffer, so get_af_specific can
 * do a proper size check in __sctp_connect
 */

Neil

>   Marcelo
> 
> > 
> > Neil
> > 
> > > > 
> > > > > 
> > > > > Neil
> > > > > 
> > > > > > >
> > > > > > > Neil
> > > > > > >
> > > > > > > >               return -EINVAL;
> > > > > > > >
> > > > > > > >       kaddrs = memdup_user(addrs, addrs_size);
> > > > > > > > --
> > > > > > > > 2.1.0
> > > > > > > >
> > > > > > > >
> > > > > > 
> > > 
> 

[PATCH net-next 2/4] sctp: clean up __sctp_connect

[Xin Long]

__sctp_connect is doing quit similar things as sctp_sendmsg_new_asoc.
To factor out common functions, this patch is to clean up their code
to make them look more similar:

  1. create the asoc and add a peer with the 1st addr.
  2. add peers with the other addrs into this asoc one by one.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 211 +++++++++++++++++++-----------------------------------
 1 file changed, 75 insertions(+), 136 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 5f92e4a..49837e9 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1049,154 +1049,108 @@ static int sctp_setsockopt_bindx(struct sock *sk,
  * Common routine for handling connect() and sctp_connectx().
  * Connect will come in with just a single address.
  */
-static int __sctp_connect(struct sock *sk,
-			  struct sockaddr *kaddrs,
-			  int addrs_size, int flags,
-			  sctp_assoc_t *assoc_id)
+static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
+			  int addrs_size, int flags, sctp_assoc_t *assoc_id)
 {
-	struct net *net = sock_net(sk);
-	struct sctp_sock *sp;
-	struct sctp_endpoint *ep;
-	struct sctp_association *asoc = NULL;
-	struct sctp_association *asoc2;
+	struct sctp_association *old, *asoc;
+	struct sctp_sock *sp = sctp_sk(sk);
+	struct sctp_endpoint *ep = sp->ep;
 	struct sctp_transport *transport;
-	union sctp_addr to;
+	struct net *net = sock_net(sk);
+	int addrcnt, walk_size, err;
+	void *addr_buf = kaddrs;
+	union sctp_addr *daddr;
 	enum sctp_scope scope;
+	struct sctp_af *af;
 	long timeo;
-	int err = 0;
-	int addrcnt = 0;
-	int walk_size = 0;
-	union sctp_addr *sa_addr = NULL;
-	void *addr_buf;
-	unsigned short port;
 
-	sp = sctp_sk(sk);
-	ep = sp->ep;
-
-	/* connect() cannot be done on a socket that is already in ESTABLISHED
-	 * state - UDP-style peeled off socket or a TCP-style socket that
-	 * is already connected.
-	 * It cannot be done even on a TCP-style listening socket.
-	 */
 	if (sctp_sstate(sk, ESTABLISHED) || sctp_sstate(sk, CLOSING) ||
-	    (sctp_style(sk, TCP) && sctp_sstate(sk, LISTENING))) {
-		err = -EISCONN;
-		goto out_free;
+	    (sctp_style(sk, TCP) && sctp_sstate(sk, LISTENING)))
+		return -EISCONN;
+
+	daddr = addr_buf;
+	af = sctp_get_af_specific(daddr->sa.sa_family);
+	if (!af || af->sockaddr_len > addrs_size)
+		return -EINVAL;
+
+	err = sctp_verify_addr(sk, daddr, af->sockaddr_len);
+	if (err)
+		return err;
+
+	asoc = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
+	if (asoc)
+		return asoc->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
+							     : -EALREADY;
+
+	if (sctp_endpoint_is_peeled_off(ep, daddr))
+		return -EADDRNOTAVAIL;
+
+	if (!ep->base.bind_addr.port) {
+		if (sctp_autobind(sk))
+			return -EAGAIN;
+	} else {
+		if (ep->base.bind_addr.port < inet_prot_sock(net) &&
+		    !ns_capable(net->user_ns, CAP_NET_BIND_SERVICE))
+			return -EACCES;
 	}
 
-	/* Walk through the addrs buffer and count the number of addresses. */
-	addr_buf = kaddrs;
-	while (walk_size < addrs_size) {
-		struct sctp_af *af;
+	scope = sctp_scope(daddr);
+	asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
+	if (!asoc)
+		return -ENOMEM;
 
-		if (walk_size + sizeof(sa_family_t) > addrs_size) {
-			err = -EINVAL;
-			goto out_free;
-		}
+	err = sctp_assoc_set_bind_addr_from_ep(asoc, scope, GFP_KERNEL);
+	if (err < 0)
+		goto out_free;
 
-		sa_addr = addr_buf;
-		af = sctp_get_af_specific(sa_addr->sa.sa_family);
+	transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL, SCTP_UNKNOWN);
+	if (!transport) {
+		err = -ENOMEM;
+		goto out_free;
+	}
 
-		/* If the address family is not supported or if this address
-		 * causes the address buffer to overflow return EINVAL.
-		 */
-		if (!af || (walk_size + af->sockaddr_len) > addrs_size) {
-			err = -EINVAL;
+	addr_buf += af->sockaddr_len;
+	walk_size = af->sockaddr_len;
+	addrcnt = 1;
+	while (walk_size < addrs_size) {
+		err = -EINVAL;
+		if (walk_size + sizeof(sa_family_t) > addrs_size)
 			goto out_free;
-		}
-
-		port = ntohs(sa_addr->v4.sin_port);
 
-		/* Save current address so we can work with it */
-		memcpy(&to, sa_addr, af->sockaddr_len);
+		daddr = addr_buf;
+		af = sctp_get_af_specific(daddr->sa.sa_family);
+		if (!af || af->sockaddr_len + walk_size > addrs_size)
+			goto out_free;
 
-		err = sctp_verify_addr(sk, &to, af->sockaddr_len);
-		if (err)
+		if (asoc->peer.port != ntohs(daddr->v4.sin_port))
 			goto out_free;
 
-		/* Make sure the destination port is correctly set
-		 * in all addresses.
-		 */
-		if (asoc && asoc->peer.port && asoc->peer.port != port) {
-			err = -EINVAL;
+		err = sctp_verify_addr(sk, daddr, af->sockaddr_len);
+		if (err)
 			goto out_free;
-		}
 
-		/* Check if there already is a matching association on the
-		 * endpoint (other than the one created here).
-		 */
-		asoc2 = sctp_endpoint_lookup_assoc(ep, &to, &transport);
-		if (asoc2 && asoc2 != asoc) {
-			if (asoc2->state >= SCTP_STATE_ESTABLISHED)
-				err = -EISCONN;
-			else
-				err = -EALREADY;
+		old = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
+		if (old && old != asoc) {
+			err = old->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
+								   : -EALREADY;
 			goto out_free;
 		}
 
-		/* If we could not find a matching association on the endpoint,
-		 * make sure that there is no peeled-off association matching
-		 * the peer address even on another socket.
-		 */
-		if (sctp_endpoint_is_peeled_off(ep, &to)) {
+		if (sctp_endpoint_is_peeled_off(ep, daddr)) {
 			err = -EADDRNOTAVAIL;
 			goto out_free;
 		}
 
-		if (!asoc) {
-			/* If a bind() or sctp_bindx() is not called prior to
-			 * an sctp_connectx() call, the system picks an
-			 * ephemeral port and will choose an address set
-			 * equivalent to binding with a wildcard address.
-			 */
-			if (!ep->base.bind_addr.port) {
-				if (sctp_autobind(sk)) {
-					err = -EAGAIN;
-					goto out_free;
-				}
-			} else {
-				/*
-				 * If an unprivileged user inherits a 1-many
-				 * style socket with open associations on a
-				 * privileged port, it MAY be permitted to
-				 * accept new associations, but it SHOULD NOT
-				 * be permitted to open new associations.
-				 */
-				if (ep->base.bind_addr.port <
-				    inet_prot_sock(net) &&
-				    !ns_capable(net->user_ns,
-				    CAP_NET_BIND_SERVICE)) {
-					err = -EACCES;
-					goto out_free;
-				}
-			}
-
-			scope = sctp_scope(&to);
-			asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
-			if (!asoc) {
-				err = -ENOMEM;
-				goto out_free;
-			}
-
-			err = sctp_assoc_set_bind_addr_from_ep(asoc, scope,
-							      GFP_KERNEL);
-			if (err < 0) {
-				goto out_free;
-			}
-
-		}
-
-		/* Prime the peer's transport structures.  */
-		transport = sctp_assoc_add_peer(asoc, &to, GFP_KERNEL,
+		transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL,
 						SCTP_UNKNOWN);
 		if (!transport) {
 			err = -ENOMEM;
 			goto out_free;
 		}
 
-		addrcnt++;
-		addr_buf += af->sockaddr_len;
+		addr_buf  += af->sockaddr_len;
 		walk_size += af->sockaddr_len;
+		addrcnt++;
 	}
 
 	/* In case the user of sctp_connectx() wants an association
@@ -1209,39 +1163,24 @@ static int __sctp_connect(struct sock *sk,
 	}
 
 	err = sctp_primitive_ASSOCIATE(net, asoc, NULL);
-	if (err < 0) {
+	if (err < 0)
 		goto out_free;
-	}
 
 	/* Initialize sk's dport and daddr for getpeername() */
 	inet_sk(sk)->inet_dport = htons(asoc->peer.port);
-	sp->pf->to_sk_daddr(sa_addr, sk);
+	sp->pf->to_sk_daddr(daddr, sk);
 	sk->sk_err = 0;
 
-	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
-
 	if (assoc_id)
 		*assoc_id = asoc->assoc_id;
 
-	err = sctp_wait_for_connect(asoc, &timeo);
-	/* Note: the asoc may be freed after the return of
-	 * sctp_wait_for_connect.
-	 */
-
-	/* Don't free association on exit. */
-	asoc = NULL;
+	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
+	return sctp_wait_for_connect(asoc, &timeo);
 
 out_free:
 	pr_debug("%s: took out_free path with asoc:%p kaddrs:%p err:%d\n",
 		 __func__, asoc, kaddrs, err);
-
-	if (asoc) {
-		/* sctp_primitive_ASSOCIATE may have added this association
-		 * To the hash table, try to unhash it, just in case, its a noop
-		 * if it wasn't hashed so we're safe
-		 */
-		sctp_association_free(asoc);
-	}
+	sctp_association_free(asoc);
 	return err;
 }
 
-- 
2.1.0

Re: [PATCH net-next 2/4] sctp: clean up __sctp_connect

[Marcelo Ricardo Leitner]

On Tue, Jul 23, 2019 at 01:37:58AM +0800, Xin Long wrote:
> __sctp_connect is doing quit similar things as sctp_sendmsg_new_asoc.
> To factor out common functions, this patch is to clean up their code
> to make them look more similar:
> 
>   1. create the asoc and add a peer with the 1st addr.
>   2. add peers with the other addrs into this asoc one by one.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/sctp/socket.c | 211 +++++++++++++++++++-----------------------------------
>  1 file changed, 75 insertions(+), 136 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 5f92e4a..49837e9 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -1049,154 +1049,108 @@ static int sctp_setsockopt_bindx(struct sock *sk,
>   * Common routine for handling connect() and sctp_connectx().
>   * Connect will come in with just a single address.
>   */
> -static int __sctp_connect(struct sock *sk,
> -			  struct sockaddr *kaddrs,
> -			  int addrs_size, int flags,
> -			  sctp_assoc_t *assoc_id)
> +static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
> +			  int addrs_size, int flags, sctp_assoc_t *assoc_id)
>  {
> -	struct net *net = sock_net(sk);
> -	struct sctp_sock *sp;
> -	struct sctp_endpoint *ep;
> -	struct sctp_association *asoc = NULL;
> -	struct sctp_association *asoc2;
> +	struct sctp_association *old, *asoc;
> +	struct sctp_sock *sp = sctp_sk(sk);
> +	struct sctp_endpoint *ep = sp->ep;
>  	struct sctp_transport *transport;
> -	union sctp_addr to;
> +	struct net *net = sock_net(sk);
> +	int addrcnt, walk_size, err;
> +	void *addr_buf = kaddrs;
> +	union sctp_addr *daddr;
>  	enum sctp_scope scope;
> +	struct sctp_af *af;
>  	long timeo;
> -	int err = 0;
> -	int addrcnt = 0;
> -	int walk_size = 0;
> -	union sctp_addr *sa_addr = NULL;
> -	void *addr_buf;
> -	unsigned short port;
>  
> -	sp = sctp_sk(sk);
> -	ep = sp->ep;
> -
> -	/* connect() cannot be done on a socket that is already in ESTABLISHED
> -	 * state - UDP-style peeled off socket or a TCP-style socket that
> -	 * is already connected.
> -	 * It cannot be done even on a TCP-style listening socket.
> -	 */
>  	if (sctp_sstate(sk, ESTABLISHED) || sctp_sstate(sk, CLOSING) ||
> -	    (sctp_style(sk, TCP) && sctp_sstate(sk, LISTENING))) {
> -		err = -EISCONN;
> -		goto out_free;
> +	    (sctp_style(sk, TCP) && sctp_sstate(sk, LISTENING)))
> +		return -EISCONN;
> +
> +	daddr = addr_buf;
> +	af = sctp_get_af_specific(daddr->sa.sa_family);
> +	if (!af || af->sockaddr_len > addrs_size)
> +		return -EINVAL;
> +
> +	err = sctp_verify_addr(sk, daddr, af->sockaddr_len);
> +	if (err)
> +		return err;
> +
> +	asoc = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
> +	if (asoc)
> +		return asoc->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
> +							     : -EALREADY;
> +
> +	if (sctp_endpoint_is_peeled_off(ep, daddr))
> +		return -EADDRNOTAVAIL;
> +
> +	if (!ep->base.bind_addr.port) {
> +		if (sctp_autobind(sk))
> +			return -EAGAIN;
> +	} else {
> +		if (ep->base.bind_addr.port < inet_prot_sock(net) &&
> +		    !ns_capable(net->user_ns, CAP_NET_BIND_SERVICE))
> +			return -EACCES;
>  	}
>  
> -	/* Walk through the addrs buffer and count the number of addresses. */
> -	addr_buf = kaddrs;
> -	while (walk_size < addrs_size) {
> -		struct sctp_af *af;
> +	scope = sctp_scope(daddr);
> +	asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
> +	if (!asoc)
> +		return -ENOMEM;
>  
> -		if (walk_size + sizeof(sa_family_t) > addrs_size) {
> -			err = -EINVAL;
> -			goto out_free;
> -		}
> +	err = sctp_assoc_set_bind_addr_from_ep(asoc, scope, GFP_KERNEL);
> +	if (err < 0)
> +		goto out_free;
>  
> -		sa_addr = addr_buf;
> -		af = sctp_get_af_specific(sa_addr->sa.sa_family);
> +	transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL, SCTP_UNKNOWN);
> +	if (!transport) {
> +		err = -ENOMEM;
> +		goto out_free;
> +	}
>  
> -		/* If the address family is not supported or if this address
> -		 * causes the address buffer to overflow return EINVAL.
> -		 */
> -		if (!af || (walk_size + af->sockaddr_len) > addrs_size) {
> -			err = -EINVAL;
> +	addr_buf += af->sockaddr_len;
> +	walk_size = af->sockaddr_len;
> +	addrcnt = 1;

This variable can be removed (in a follow-up patch). It's only
incremented and never used other than that.

> +	while (walk_size < addrs_size) {
> +		err = -EINVAL;
> +		if (walk_size + sizeof(sa_family_t) > addrs_size)
>  			goto out_free;
> -		}
> -
> -		port = ntohs(sa_addr->v4.sin_port);
>  
> -		/* Save current address so we can work with it */
> -		memcpy(&to, sa_addr, af->sockaddr_len);
> +		daddr = addr_buf;
> +		af = sctp_get_af_specific(daddr->sa.sa_family);
> +		if (!af || af->sockaddr_len + walk_size > addrs_size)
> +			goto out_free;
>  
> -		err = sctp_verify_addr(sk, &to, af->sockaddr_len);
> -		if (err)
> +		if (asoc->peer.port != ntohs(daddr->v4.sin_port))
>  			goto out_free;
>  
> -		/* Make sure the destination port is correctly set
> -		 * in all addresses.
> -		 */
> -		if (asoc && asoc->peer.port && asoc->peer.port != port) {
> -			err = -EINVAL;
> +		err = sctp_verify_addr(sk, daddr, af->sockaddr_len);
> +		if (err)
>  			goto out_free;
> -		}
>  
> -		/* Check if there already is a matching association on the
> -		 * endpoint (other than the one created here).
> -		 */
> -		asoc2 = sctp_endpoint_lookup_assoc(ep, &to, &transport);
> -		if (asoc2 && asoc2 != asoc) {
> -			if (asoc2->state >= SCTP_STATE_ESTABLISHED)
> -				err = -EISCONN;
> -			else
> -				err = -EALREADY;
> +		old = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
> +		if (old && old != asoc) {
> +			err = old->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
> +								   : -EALREADY;
>  			goto out_free;
>  		}
>  
> -		/* If we could not find a matching association on the endpoint,
> -		 * make sure that there is no peeled-off association matching
> -		 * the peer address even on another socket.
> -		 */
> -		if (sctp_endpoint_is_peeled_off(ep, &to)) {
> +		if (sctp_endpoint_is_peeled_off(ep, daddr)) {
>  			err = -EADDRNOTAVAIL;
>  			goto out_free;
>  		}
>  
> -		if (!asoc) {
> -			/* If a bind() or sctp_bindx() is not called prior to
> -			 * an sctp_connectx() call, the system picks an
> -			 * ephemeral port and will choose an address set
> -			 * equivalent to binding with a wildcard address.
> -			 */
> -			if (!ep->base.bind_addr.port) {
> -				if (sctp_autobind(sk)) {
> -					err = -EAGAIN;
> -					goto out_free;
> -				}
> -			} else {
> -				/*
> -				 * If an unprivileged user inherits a 1-many
> -				 * style socket with open associations on a
> -				 * privileged port, it MAY be permitted to
> -				 * accept new associations, but it SHOULD NOT
> -				 * be permitted to open new associations.
> -				 */
> -				if (ep->base.bind_addr.port <
> -				    inet_prot_sock(net) &&
> -				    !ns_capable(net->user_ns,
> -				    CAP_NET_BIND_SERVICE)) {
> -					err = -EACCES;
> -					goto out_free;
> -				}
> -			}
> -
> -			scope = sctp_scope(&to);
> -			asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
> -			if (!asoc) {
> -				err = -ENOMEM;
> -				goto out_free;
> -			}
> -
> -			err = sctp_assoc_set_bind_addr_from_ep(asoc, scope,
> -							      GFP_KERNEL);
> -			if (err < 0) {
> -				goto out_free;
> -			}
> -
> -		}
> -
> -		/* Prime the peer's transport structures.  */
> -		transport = sctp_assoc_add_peer(asoc, &to, GFP_KERNEL,
> +		transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL,
>  						SCTP_UNKNOWN);
>  		if (!transport) {
>  			err = -ENOMEM;
>  			goto out_free;
>  		}
>  
> -		addrcnt++;
> -		addr_buf += af->sockaddr_len;
> +		addr_buf  += af->sockaddr_len;
>  		walk_size += af->sockaddr_len;
> +		addrcnt++;
>  	}
>  
>  	/* In case the user of sctp_connectx() wants an association
> @@ -1209,39 +1163,24 @@ static int __sctp_connect(struct sock *sk,
>  	}
>  
>  	err = sctp_primitive_ASSOCIATE(net, asoc, NULL);
> -	if (err < 0) {
> +	if (err < 0)
>  		goto out_free;
> -	}
>  
>  	/* Initialize sk's dport and daddr for getpeername() */
>  	inet_sk(sk)->inet_dport = htons(asoc->peer.port);
> -	sp->pf->to_sk_daddr(sa_addr, sk);
> +	sp->pf->to_sk_daddr(daddr, sk);
>  	sk->sk_err = 0;
>  
> -	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> -
>  	if (assoc_id)
>  		*assoc_id = asoc->assoc_id;
>  
> -	err = sctp_wait_for_connect(asoc, &timeo);
> -	/* Note: the asoc may be freed after the return of
> -	 * sctp_wait_for_connect.
> -	 */
> -
> -	/* Don't free association on exit. */
> -	asoc = NULL;
> +	timeo = sock_sndtimeo(sk, flags & O_NONBLOCK);
> +	return sctp_wait_for_connect(asoc, &timeo);
>  
>  out_free:
>  	pr_debug("%s: took out_free path with asoc:%p kaddrs:%p err:%d\n",
>  		 __func__, asoc, kaddrs, err);
> -
> -	if (asoc) {
> -		/* sctp_primitive_ASSOCIATE may have added this association
> -		 * To the hash table, try to unhash it, just in case, its a noop
> -		 * if it wasn't hashed so we're safe
> -		 */
> -		sctp_association_free(asoc);
> -	}
> +	sctp_association_free(asoc);
>  	return err;
>  }
>  
> -- 
> 2.1.0
> 

[PATCH net-next 3/4] sctp: factor out sctp_connect_new_asoc

[Xin Long]

In this function factored out from sctp_sendmsg_new_asoc() and
__sctp_connect(), it creates the asoc and adds a peer with the
1st addr.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 160 ++++++++++++++++++++++++++----------------------------
 1 file changed, 76 insertions(+), 84 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 49837e9..420abdb 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1044,6 +1044,73 @@ static int sctp_setsockopt_bindx(struct sock *sk,
 	return err;
 }
 
+static int sctp_connect_new_asoc(struct sctp_endpoint *ep,
+				 const union sctp_addr *daddr,
+				 const struct sctp_initmsg *init,
+				 struct sctp_transport **tp)
+{
+	struct sctp_association *asoc;
+	struct sock *sk = ep->base.sk;
+	struct net *net = sock_net(sk);
+	enum sctp_scope scope;
+	int err;
+
+	if (sctp_endpoint_is_peeled_off(ep, daddr))
+		return -EADDRNOTAVAIL;
+
+	if (!ep->base.bind_addr.port) {
+		if (sctp_autobind(sk))
+			return -EAGAIN;
+	} else {
+		if (ep->base.bind_addr.port < inet_prot_sock(net) &&
+		    !ns_capable(net->user_ns, CAP_NET_BIND_SERVICE))
+			return -EACCES;
+	}
+
+	scope = sctp_scope(daddr);
+	asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
+	if (!asoc)
+		return -ENOMEM;
+
+	err = sctp_assoc_set_bind_addr_from_ep(asoc, scope, GFP_KERNEL);
+	if (err < 0)
+		goto free;
+
+	*tp = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL, SCTP_UNKNOWN);
+	if (!*tp) {
+		err = -ENOMEM;
+		goto free;
+	}
+
+	if (!init)
+		return 0;
+
+	if (init->sinit_num_ostreams) {
+		__u16 outcnt = init->sinit_num_ostreams;
+
+		asoc->c.sinit_num_ostreams = outcnt;
+		/* outcnt has been changed, need to re-init stream */
+		err = sctp_stream_init(&asoc->stream, outcnt, 0, GFP_KERNEL);
+		if (err)
+			goto free;
+	}
+
+	if (init->sinit_max_instreams)
+		asoc->c.sinit_max_instreams = init->sinit_max_instreams;
+
+	if (init->sinit_max_attempts)
+		asoc->max_init_attempts = init->sinit_max_attempts;
+
+	if (init->sinit_max_init_timeo)
+		asoc->max_init_timeo =
+			msecs_to_jiffies(init->sinit_max_init_timeo);
+
+	return 0;
+free:
+	sctp_association_free(asoc);
+	return err;
+}
+
 /* __sctp_connect(struct sock* sk, struct sockaddr *kaddrs, int addrs_size)
  *
  * Common routine for handling connect() and sctp_connectx().
@@ -1056,11 +1123,9 @@ static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
 	struct sctp_sock *sp = sctp_sk(sk);
 	struct sctp_endpoint *ep = sp->ep;
 	struct sctp_transport *transport;
-	struct net *net = sock_net(sk);
 	int addrcnt, walk_size, err;
 	void *addr_buf = kaddrs;
 	union sctp_addr *daddr;
-	enum sctp_scope scope;
 	struct sctp_af *af;
 	long timeo;
 
@@ -1082,32 +1147,10 @@ static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
 		return asoc->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
 							     : -EALREADY;
 
-	if (sctp_endpoint_is_peeled_off(ep, daddr))
-		return -EADDRNOTAVAIL;
-
-	if (!ep->base.bind_addr.port) {
-		if (sctp_autobind(sk))
-			return -EAGAIN;
-	} else {
-		if (ep->base.bind_addr.port < inet_prot_sock(net) &&
-		    !ns_capable(net->user_ns, CAP_NET_BIND_SERVICE))
-			return -EACCES;
-	}
-
-	scope = sctp_scope(daddr);
-	asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
-	if (!asoc)
-		return -ENOMEM;
-
-	err = sctp_assoc_set_bind_addr_from_ep(asoc, scope, GFP_KERNEL);
-	if (err < 0)
-		goto out_free;
-
-	transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL, SCTP_UNKNOWN);
-	if (!transport) {
-		err = -ENOMEM;
-		goto out_free;
-	}
+	err = sctp_connect_new_asoc(ep, daddr, NULL, &transport);
+	if (err)
+		return err;
+	asoc = transport->asoc;
 
 	addr_buf += af->sockaddr_len;
 	walk_size = af->sockaddr_len;
@@ -1162,7 +1205,7 @@ static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
 			goto out_free;
 	}
 
-	err = sctp_primitive_ASSOCIATE(net, asoc, NULL);
+	err = sctp_primitive_ASSOCIATE(sock_net(sk), asoc, NULL);
 	if (err < 0)
 		goto out_free;
 
@@ -1598,9 +1641,7 @@ static int sctp_sendmsg_new_asoc(struct sock *sk, __u16 sflags,
 				 struct sctp_transport **tp)
 {
 	struct sctp_endpoint *ep = sctp_sk(sk)->ep;
-	struct net *net = sock_net(sk);
 	struct sctp_association *asoc;
-	enum sctp_scope scope;
 	struct cmsghdr *cmsg;
 	__be32 flowinfo = 0;
 	struct sctp_af *af;
@@ -1615,20 +1656,6 @@ static int sctp_sendmsg_new_asoc(struct sock *sk, __u16 sflags,
 				    sctp_sstate(sk, CLOSING)))
 		return -EADDRNOTAVAIL;
 
-	if (sctp_endpoint_is_peeled_off(ep, daddr))
-		return -EADDRNOTAVAIL;
-
-	if (!ep->base.bind_addr.port) {
-		if (sctp_autobind(sk))
-			return -EAGAIN;
-	} else {
-		if (ep->base.bind_addr.port < inet_prot_sock(net) &&
-		    !ns_capable(net->user_ns, CAP_NET_BIND_SERVICE))
-			return -EACCES;
-	}
-
-	scope = sctp_scope(daddr);
-
 	/* Label connection socket for first association 1-to-many
 	 * style for client sequence socket()->sendmsg(). This
 	 * needs to be done before sctp_assoc_add_peer() as that will
@@ -1644,45 +1671,10 @@ static int sctp_sendmsg_new_asoc(struct sock *sk, __u16 sflags,
 	if (err < 0)
 		return err;
 
-	asoc = sctp_association_new(ep, sk, scope, GFP_KERNEL);
-	if (!asoc)
-		return -ENOMEM;
-
-	if (sctp_assoc_set_bind_addr_from_ep(asoc, scope, GFP_KERNEL) < 0) {
-		err = -ENOMEM;
-		goto free;
-	}
-
-	if (cmsgs->init) {
-		struct sctp_initmsg *init = cmsgs->init;
-
-		if (init->sinit_num_ostreams) {
-			__u16 outcnt = init->sinit_num_ostreams;
-
-			asoc->c.sinit_num_ostreams = outcnt;
-			/* outcnt has been changed, need to re-init stream */
-			err = sctp_stream_init(&asoc->stream, outcnt, 0,
-					       GFP_KERNEL);
-			if (err)
-				goto free;
-		}
-
-		if (init->sinit_max_instreams)
-			asoc->c.sinit_max_instreams = init->sinit_max_instreams;
-
-		if (init->sinit_max_attempts)
-			asoc->max_init_attempts = init->sinit_max_attempts;
-
-		if (init->sinit_max_init_timeo)
-			asoc->max_init_timeo =
-				msecs_to_jiffies(init->sinit_max_init_timeo);
-	}
-
-	*tp = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL, SCTP_UNKNOWN);
-	if (!*tp) {
-		err = -ENOMEM;
-		goto free;
-	}
+	err = sctp_connect_new_asoc(ep, daddr, cmsgs->init, tp);
+	if (err)
+		return err;
+	asoc = (*tp)->asoc;
 
 	if (!cmsgs->addrs_msg)
 		return 0;
-- 
2.1.0

[PATCH net-next 4/4] sctp: factor out sctp_connect_add_peer

[Xin Long]

In this function factored out from sctp_sendmsg_new_asoc() and
__sctp_connect(), it adds a peer with the other addr into the
asoc after this asoc is created with the 1st addr.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sctp/socket.c | 76 +++++++++++++++++++++++--------------------------------
 1 file changed, 31 insertions(+), 45 deletions(-)

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 420abdb..6584c19 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -1111,6 +1111,33 @@ static int sctp_connect_new_asoc(struct sctp_endpoint *ep,
 	return err;
 }
 
+static int sctp_connect_add_peer(struct sctp_association *asoc,
+				 union sctp_addr *daddr, int addr_len)
+{
+	struct sctp_endpoint *ep = asoc->ep;
+	struct sctp_association *old;
+	struct sctp_transport *t;
+	int err;
+
+	err = sctp_verify_addr(ep->base.sk, daddr, addr_len);
+	if (err)
+		return err;
+
+	old = sctp_endpoint_lookup_assoc(ep, daddr, &t);
+	if (old && old != asoc)
+		return old->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
+							    : -EALREADY;
+
+	if (sctp_endpoint_is_peeled_off(ep, daddr))
+		return -EADDRNOTAVAIL;
+
+	t = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL, SCTP_UNKNOWN);
+	if (!t)
+		return -ENOMEM;
+
+	return 0;
+}
+
 /* __sctp_connect(struct sock* sk, struct sockaddr *kaddrs, int addrs_size)
  *
  * Common routine for handling connect() and sctp_connectx().
@@ -1119,10 +1146,10 @@ static int sctp_connect_new_asoc(struct sctp_endpoint *ep,
 static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
 			  int addrs_size, int flags, sctp_assoc_t *assoc_id)
 {
-	struct sctp_association *old, *asoc;
 	struct sctp_sock *sp = sctp_sk(sk);
 	struct sctp_endpoint *ep = sp->ep;
 	struct sctp_transport *transport;
+	struct sctp_association *asoc;
 	int addrcnt, walk_size, err;
 	void *addr_buf = kaddrs;
 	union sctp_addr *daddr;
@@ -1168,29 +1195,10 @@ static int __sctp_connect(struct sock *sk, struct sockaddr *kaddrs,
 		if (asoc->peer.port != ntohs(daddr->v4.sin_port))
 			goto out_free;
 
-		err = sctp_verify_addr(sk, daddr, af->sockaddr_len);
+		err = sctp_connect_add_peer(asoc, daddr, af->sockaddr_len);
 		if (err)
 			goto out_free;
 
-		old = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
-		if (old && old != asoc) {
-			err = old->state >= SCTP_STATE_ESTABLISHED ? -EISCONN
-								   : -EALREADY;
-			goto out_free;
-		}
-
-		if (sctp_endpoint_is_peeled_off(ep, daddr)) {
-			err = -EADDRNOTAVAIL;
-			goto out_free;
-		}
-
-		transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL,
-						SCTP_UNKNOWN);
-		if (!transport) {
-			err = -ENOMEM;
-			goto out_free;
-		}
-
 		addr_buf  += af->sockaddr_len;
 		walk_size += af->sockaddr_len;
 		addrcnt++;
@@ -1684,8 +1692,6 @@ static int sctp_sendmsg_new_asoc(struct sock *sk, __u16 sflags,
 
 	/* sendv addr list parse */
 	for_each_cmsghdr(cmsg, cmsgs->addrs_msg) {
-		struct sctp_transport *transport;
-		struct sctp_association *old;
 		union sctp_addr _daddr;
 		int dlen;
 
@@ -1719,30 +1725,10 @@ static int sctp_sendmsg_new_asoc(struct sock *sk, __u16 sflags,
 			daddr->v6.sin6_port = htons(asoc->peer.port);
 			memcpy(&daddr->v6.sin6_addr, CMSG_DATA(cmsg), dlen);
 		}
-		err = sctp_verify_addr(sk, daddr, sizeof(*daddr));
-		if (err)
-			goto free;
-
-		old = sctp_endpoint_lookup_assoc(ep, daddr, &transport);
-		if (old && old != asoc) {
-			if (old->state >= SCTP_STATE_ESTABLISHED)
-				err = -EISCONN;
-			else
-				err = -EALREADY;
-			goto free;
-		}
 
-		if (sctp_endpoint_is_peeled_off(ep, daddr)) {
-			err = -EADDRNOTAVAIL;
-			goto free;
-		}
-
-		transport = sctp_assoc_add_peer(asoc, daddr, GFP_KERNEL,
-						SCTP_UNKNOWN);
-		if (!transport) {
-			err = -ENOMEM;
+		err = sctp_connect_add_peer(asoc, daddr, sizeof(*daddr));
+		if (err)
 			goto free;
-		}
 	}
 
 	return 0;
-- 
2.1.0

Re: [PATCH net-next 0/4] sctp: clean up __sctp_connect function

[Marcelo Ricardo Leitner]

On Tue, Jul 23, 2019 at 01:37:56AM +0800, Xin Long wrote:
> This patchset is to factor out some common code for
> sctp_sendmsg_new_asoc() and __sctp_connect() into 2
> new functioins.
> 
> Xin Long (4):
>   sctp: check addr_size with sa_family_t size in
>     __sctp_setsockopt_connectx
>   sctp: clean up __sctp_connect
>   sctp: factor out sctp_connect_new_asoc
>   sctp: factor out sctp_connect_add_peer

Nice cleanup! These patches LGTM. Hopefully for Neil as well.

Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>

Re: [PATCH net-next 0/4] sctp: clean up __sctp_connect function

[Neil Horman]

On Wed, Jul 24, 2019 at 11:25:12AM -0300, Marcelo Ricardo Leitner wrote:
> On Tue, Jul 23, 2019 at 01:37:56AM +0800, Xin Long wrote:
> > This patchset is to factor out some common code for
> > sctp_sendmsg_new_asoc() and __sctp_connect() into 2
> > new functioins.
> > 
> > Xin Long (4):
> >   sctp: check addr_size with sa_family_t size in
> >     __sctp_setsockopt_connectx
> >   sctp: clean up __sctp_connect
> >   sctp: factor out sctp_connect_new_asoc
> >   sctp: factor out sctp_connect_add_peer
> 
> Nice cleanup! These patches LGTM. Hopefully for Neil as well.
> 
> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> 

Yes, agreed, this all looks good, but I would like to resolve the addr_length
check issue before I ack it.
Neil

Re: [PATCH net-next 0/4] sctp: clean up __sctp_connect function

[David Miller]

From: Neil Horman <nhorman@tuxdriver.com>
Date: Wed, 24 Jul 2019 14:47:29 -0400

> On Wed, Jul 24, 2019 at 11:25:12AM -0300, Marcelo Ricardo Leitner wrote:
>> On Tue, Jul 23, 2019 at 01:37:56AM +0800, Xin Long wrote:
>> > This patchset is to factor out some common code for
>> > sctp_sendmsg_new_asoc() and __sctp_connect() into 2
>> > new functioins.
>> > 
>> > Xin Long (4):
>> >   sctp: check addr_size with sa_family_t size in
>> >     __sctp_setsockopt_connectx
>> >   sctp: clean up __sctp_connect
>> >   sctp: factor out sctp_connect_new_asoc
>> >   sctp: factor out sctp_connect_add_peer
>> 
>> Nice cleanup! These patches LGTM. Hopefully for Neil as well.
>> 
>> Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
>> 
> 
> Yes, agreed, this all looks good, but I would like to resolve the addr_length
> check issue before I ack it.

Once that's resolved please just respin this series with Marcelo's ACK
retained in the series introduction email.