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

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

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.