netdev - Re: [PATCH net-next] sctp: Pull the user copies out of the individual sockopt functions.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 21 May 2020 13:45:04 -0300
From:   'Marcelo Ricardo Leitner' <marcelo.leitner@...il.com>
To:     David Laight <David.Laight@...lab.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
        Neil Horman <nhorman@...driver.com>,
        Christoph Hellwig <hch@....de>
Subject: Re: [PATCH net-next] sctp: Pull the user copies out of the
 individual sockopt functions.

On Thu, May 21, 2020 at 04:09:15PM +0000, David Laight wrote:
> From: 'Marcelo Ricardo Leitner'
> > Sent: 21 May 2020 16:37
> > On Wed, May 20, 2020 at 03:08:13PM +0000, David Laight wrote:
> ...
> > > Only SCTP_SOCKOPT_CONNECTX3 contains an indirect pointer.
> > > It is also the only getsockopt() that wants to return a buffer
> > > and an error code. It is also definitely abusing getsockopt().
> > 
> > It should have been a linear buffer. The secondary __user access is
> > way worse than having the application to do another allocation. But
> > too late..
> 
> I think that is SCTP_SOCKOPT_CONNECTX ?

Right :-)

...
> > > +	if (optlen < sizeof (param_buf)) {
> > > +		if (copy_from_user(&param_buf, u_optval, optlen))
> > > +			return -EFAULT;
> > > +		optval = param_buf;
> > > +	} else {
> > > +		if (optlen > USHRT_MAX)
> > > +			optlen = USHRT_MAX;
> > 
> > There are functions that can work with and expect buffers larger than
> > that, such as sctp_setsockopt_auth_key:
> 
> I'd assumed the maximums were silly.
> But a few more than 64k is enough, the lengths are in bytes.
> OTOH 128k is a nice round limit - and plenty AFAICT.

LGTM too.

> 
> ...
> > > +	if (len < sizeof (param_buf)) {
> > > +		/* Zero first bytes to stop KASAN complaining. */
> > > +		param_buf[0] = 0;
> > > +		if (copy_from_user(&param_buf, u_optval, len))
> > > +			return -EFAULT;
> > > +		optval = param_buf;
> > > +	} else {
> > > +		if (len > USHRT_MAX)
> > > +			len = USHRT_MAX;
> > 
> > This limit is not present today for sctp_getsockopt_local_addrs()
> > calls (there may be others).  As is, it will limit it and may mean
> > that it can't dump all addresses.  We have discussed this and didn't
> > come to a conclusion on what is a safe limit to use here, at least not
> > on that time.
> 
> It needs some limit. memdup_user() might limit at 32MB.
> I couldn't decide is some of the allocators limit it further.
> In any case an IPv6 address is what? under 128 bytes.
> 64k is 512 address, things are going to explode elsewhere first.

If it does, we probably can fix that too.

> 
> I didn't see 'get' requests that did 64k + a bit.
> 
> It should be possible to loop using a larger kernel buffer if the
> data won't fit.
> Doable as a later patch to avoid complications.

Sounds complicated. 128k should be more than enough here as well.
sctp_getsockopt_local_addrs() will adjust the output to fit on the
buffer. Point being, with enough buffer, it will support the limits
the RFC states, and if the user supplies a smaller buffer, it will
dump what it can. If the user pass a larger buffer, it doesn't need
it, and it's safe to ignore the rest of the buffer (as the patch is
doing here). I didn't check the other functions now, though.