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

[Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>]

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
> > > >
> > > >
> >