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] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 13 Nov 2017 17:54:03 -0200
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     Neil Horman <nhorman@...driver.com>,
        network dev <netdev@...r.kernel.org>,
        linux-sctp@...r.kernel.org, davem <davem@...emloft.net>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: [PATCH net] sctp: check stream reset info len before making
 reconf chunk

On Mon, Nov 13, 2017 at 11:15:40PM +0800, Xin Long wrote:
> On Mon, Nov 13, 2017 at 11:09 PM, Neil Horman <nhorman@...driver.com> wrote:
> > On Mon, Nov 13, 2017 at 01:39:27PM +0800, Xin Long wrote:
> >> Now when resetting stream, if both in and out flags are set, the info
> >> len can reach:
> >>   sizeof(struct sctp_strreset_outreq) + SCTP_MAX_STREAM(65535) +
> >>   sizeof(struct sctp_strreset_inreq)  + SCTP_MAX_STREAM(65535)
> >> even without duplicated stream no, this value is far greater than the
> >> chunk's max size.
> >>
> >> _sctp_make_chunk doesn't do any check for this, which would cause the
> >> skb it allocs is huge, syzbot even reported a crash due to this.
> >>
> >> This patch is to check stream reset info len before making reconf
> >> chunk and return NULL if the len exceeds chunk's capacity.
> >>
> >> Fixes: cc16f00f6529 ("sctp: add support for generating stream reconf ssn reset request chunk")
> >> Reported-by: Dmitry Vyukov <dvyukov@...gle.com>
> >> Signed-off-by: Xin Long <lucien.xin@...il.com>
> >> ---
> >>  net/sctp/sm_make_chunk.c | 7 +++++--
> >>  net/sctp/stream.c        | 8 +++++---
> >>  2 files changed, 10 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> >> index 514465b..a21328a 100644
> >> --- a/net/sctp/sm_make_chunk.c
> >> +++ b/net/sctp/sm_make_chunk.c
> >> @@ -3598,14 +3598,17 @@ struct sctp_chunk *sctp_make_strreset_req(
> >>       __u16 stream_len = stream_num * 2;

Unrelated, but.. won't stream_len overflow if stream_num >= 32768?
When called form sctp_send_reset_streams() I don't see anything
restricting it to such range.

> >>       struct sctp_strreset_inreq inreq;
> >>       struct sctp_chunk *retval;
> >> -     __u16 outlen, inlen;
> >> +     int outlen, inlen;
> >>
> >>       outlen = (sizeof(outreq) + stream_len) * out;
> >>       inlen = (sizeof(inreq) + stream_len) * in;
> >>
> >> +     if (outlen + inlen > SCTP_MAX_CHUNK_LEN - sizeof(struct sctp_chunkhdr))
> >> +             return ERR_PTR(-EINVAL);
> >> +
> > Why all the ERR_PTR manipulations here?  Just returning NULL, like the fuction
> > has been doing is sufficient to set ENOMEM at both call sites
> I don't like ERR_PTR handling here either,
> But it shouldn't be ENOMEM, should it ?
> 
> It may confuse users, but I'm also ok to let it just return
> ENOMEM as you wish. wdyt ?

Returning ENOMEM in the above error can be misleading. It's not that
we cannot allocate it, it's that it won't fit the packet no matter how
much memory we add to the system.

  Marcelo

Powered by blists - more mailing lists