From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> To: Xin Long <lucien.xin@gmail.com> Cc: Neil Horman <nhorman@tuxdriver.com>, network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org, davem <davem@davemloft.net>, Dmitry Vyukov <dvyukov@google.com>, syzkaller <syzkaller@googlegroups.com> Subject: Re: [PATCH net] sctp: check stream reset info len before making reconf chunk Date: Mon, 13 Nov 2017 17:54:03 -0200 [thread overview] Message-ID: <20171113195402.GG3675@localhost.localdomain> (raw) In-Reply-To: <CADvbK_debq8umStFwEiBrMj09X31TC+36_s41bTive9Pa8Ohaw@mail.gmail.com> 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@tuxdriver.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@google.com> > >> Signed-off-by: Xin Long <lucien.xin@gmail.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
WARNING: multiple messages have this Message-ID (diff)
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> To: Xin Long <lucien.xin@gmail.com> Cc: Neil Horman <nhorman@tuxdriver.com>, network dev <netdev@vger.kernel.org>, linux-sctp@vger.kernel.org, davem <davem@davemloft.net>, Dmitry Vyukov <dvyukov@google.com>, syzkaller <syzkaller@googlegroups.com> Subject: Re: [PATCH net] sctp: check stream reset info len before making reconf chunk Date: Mon, 13 Nov 2017 19:54:03 +0000 [thread overview] Message-ID: <20171113195402.GG3675@localhost.localdomain> (raw) In-Reply-To: <CADvbK_debq8umStFwEiBrMj09X31TC+36_s41bTive9Pa8Ohaw@mail.gmail.com> 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@tuxdriver.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@google.com> > >> Signed-off-by: Xin Long <lucien.xin@gmail.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
next prev parent reply other threads:[~2017-11-13 19:54 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-11-13 5:39 [PATCH net] sctp: check stream reset info len before making reconf chunk Xin Long 2017-11-13 5:39 ` Xin Long 2017-11-13 15:09 ` Neil Horman 2017-11-13 15:09 ` Neil Horman 2017-11-13 15:15 ` Xin Long 2017-11-13 15:15 ` Xin Long 2017-11-13 19:54 ` Marcelo Ricardo Leitner [this message] 2017-11-13 19:54 ` Marcelo Ricardo Leitner 2017-11-14 8:27 ` Xin Long 2017-11-14 8:27 ` Xin Long 2017-11-14 19:06 ` Marcelo Ricardo Leitner 2017-11-14 19:06 ` Marcelo Ricardo Leitner 2017-11-14 12:46 ` Neil Horman 2017-11-14 12:46 ` Neil Horman 2017-11-14 12:57 ` Xin Long 2017-11-14 12:57 ` Xin Long
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=20171113195402.GG3675@localhost.localdomain \ --to=marcelo.leitner@gmail.com \ --cc=davem@davemloft.net \ --cc=dvyukov@google.com \ --cc=linux-sctp@vger.kernel.org \ --cc=lucien.xin@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=nhorman@tuxdriver.com \ --cc=syzkaller@googlegroups.com \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.