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(¶m_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(¶m_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.
