From: Neil Horman <nhorman@tuxdriver.com>
To: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Cc: Xin Long <lucien.xin@gmail.com>,
network dev <netdev@vger.kernel.org>,
linux-sctp@vger.kernel.org, davem <davem@davemloft.net>
Subject: Re: [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx
Date: Wed, 24 Jul 2019 16:41:12 -0400 [thread overview]
Message-ID: <20190724204112.GE7212@hmswarspite.think-freely.org> (raw)
In-Reply-To: <20190724190543.GH6204@localhost.localdomain>
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
> > > > > > > >
> > > > > > > >
> > > > > >
> > >
>
next prev parent reply other threads:[~2019-07-24 20:41 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-22 17:37 [PATCH net-next 0/4] sctp: clean up __sctp_connect function Xin Long
2019-07-22 17:37 ` [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx Xin Long
2019-07-22 17:37 ` [PATCH net-next 2/4] sctp: clean up __sctp_connect Xin Long
2019-07-22 17:37 ` [PATCH net-next 3/4] sctp: factor out sctp_connect_new_asoc Xin Long
2019-07-22 17:38 ` [PATCH net-next 4/4] sctp: factor out sctp_connect_add_peer Xin Long
2019-07-24 14:09 ` [PATCH net-next 2/4] sctp: clean up __sctp_connect Marcelo Ricardo Leitner
2019-07-23 15:24 ` [PATCH net-next 1/4] sctp: check addr_size with sa_family_t size in __sctp_setsockopt_connectx Neil Horman
2019-07-24 7:21 ` Xin Long
2019-07-24 11:22 ` Neil Horman
2019-07-24 12:36 ` Marcelo Ricardo Leitner
2019-07-24 12:49 ` Marcelo Ricardo Leitner
2019-07-24 18:44 ` Neil Horman
2019-07-24 19:05 ` Marcelo Ricardo Leitner
2019-07-24 19:12 ` Marcelo Ricardo Leitner
2019-07-24 20:43 ` Neil Horman
2019-07-26 9:11 ` Xin Long
2019-07-24 20:41 ` Neil Horman [this message]
2019-07-24 14:25 ` [PATCH net-next 0/4] sctp: clean up __sctp_connect function Marcelo Ricardo Leitner
2019-07-24 18:47 ` Neil Horman
2019-07-24 20:11 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190724204112.GE7212@hmswarspite.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=davem@davemloft.net \
--cc=linux-sctp@vger.kernel.org \
--cc=lucien.xin@gmail.com \
--cc=marcelo.leitner@gmail.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).