[PATCH net] sctp: check stream reset info len before making reconf chunk

[PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

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;
 	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);
+
 	retval = sctp_make_reconf(asoc, outlen + inlen);
 	if (!retval)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	if (outlen) {
 		outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index fa8371f..51a25bf 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
 
 	kfree(nstr_list);
 
-	if (!chunk) {
-		retval = -ENOMEM;
+	if (IS_ERR(chunk)) {
+		retval = PTR_ERR(chunk);
 		goto out;
 	}
 
@@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 	}
 
 	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
-	if (!chunk)
+	if (IS_ERR(chunk)) {
+		chunk = NULL;
 		goto out;
+	}
 
 	if (nums)
 		for (i = 0; i < nums; i++)
-- 
2.1.0

[PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

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;
 	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);
+
 	retval = sctp_make_reconf(asoc, outlen + inlen);
 	if (!retval)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	if (outlen) {
 		outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
diff --git a/net/sctp/stream.c b/net/sctp/stream.c
index fa8371f..51a25bf 100644
--- a/net/sctp/stream.c
+++ b/net/sctp/stream.c
@@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
 
 	kfree(nstr_list);
 
-	if (!chunk) {
-		retval = -ENOMEM;
+	if (IS_ERR(chunk)) {
+		retval = PTR_ERR(chunk);
 		goto out;
 	}
 
@@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
 	}
 
 	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
-	if (!chunk)
+	if (IS_ERR(chunk)) {
+		chunk = NULL;
 		goto out;
+	}
 
 	if (nums)
 		for (i = 0; i < nums; i++)
-- 
2.1.0

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Neil Horman]

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;
>  	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

Neil

>  	retval = sctp_make_reconf(asoc, outlen + inlen);
>  	if (!retval)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	if (outlen) {
>  		outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index fa8371f..51a25bf 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>  
>  	kfree(nstr_list);
>  
> -	if (!chunk) {
> -		retval = -ENOMEM;
> +	if (IS_ERR(chunk)) {
> +		retval = PTR_ERR(chunk);
>  		goto out;
>  	}
>  
> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>  	}
>  
>  	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
> -	if (!chunk)
> +	if (IS_ERR(chunk)) {
> +		chunk = NULL;
>  		goto out;
> +	}
>  
>  	if (nums)
>  		for (i = 0; i < nums; i++)
> -- 
> 2.1.0
> 
> 

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Neil Horman]

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;
>  	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

Neil

>  	retval = sctp_make_reconf(asoc, outlen + inlen);
>  	if (!retval)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	if (outlen) {
>  		outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> index fa8371f..51a25bf 100644
> --- a/net/sctp/stream.c
> +++ b/net/sctp/stream.c
> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>  
>  	kfree(nstr_list);
>  
> -	if (!chunk) {
> -		retval = -ENOMEM;
> +	if (IS_ERR(chunk)) {
> +		retval = PTR_ERR(chunk);
>  		goto out;
>  	}
>  
> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>  	}
>  
>  	chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
> -	if (!chunk)
> +	if (IS_ERR(chunk)) {
> +		chunk = NULL;
>  		goto out;
> +	}
>  
>  	if (nums)
>  		for (i = 0; i < nums; i++)
> -- 
> 2.1.0
> 
> 

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

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;
>>       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 ?

>
> Neil
>
>>       retval = sctp_make_reconf(asoc, outlen + inlen);
>>       if (!retval)
>> -             return NULL;
>> +             return ERR_PTR(-ENOMEM);
>>
>>       if (outlen) {
>>               outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
>> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> index fa8371f..51a25bf 100644
>> --- a/net/sctp/stream.c
>> +++ b/net/sctp/stream.c
>> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>>
>>       kfree(nstr_list);
>>
>> -     if (!chunk) {
>> -             retval = -ENOMEM;
>> +     if (IS_ERR(chunk)) {
>> +             retval = PTR_ERR(chunk);
>>               goto out;
>>       }
>>
>> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>>       }
>>
>>       chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
>> -     if (!chunk)
>> +     if (IS_ERR(chunk)) {
>> +             chunk = NULL;
>>               goto out;
>> +     }
>>
>>       if (nums)
>>               for (i = 0; i < nums; i++)
>> --
>> 2.1.0
>>
>>

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

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;
>>       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 ?

>
> Neil
>
>>       retval = sctp_make_reconf(asoc, outlen + inlen);
>>       if (!retval)
>> -             return NULL;
>> +             return ERR_PTR(-ENOMEM);
>>
>>       if (outlen) {
>>               outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
>> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> index fa8371f..51a25bf 100644
>> --- a/net/sctp/stream.c
>> +++ b/net/sctp/stream.c
>> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>>
>>       kfree(nstr_list);
>>
>> -     if (!chunk) {
>> -             retval = -ENOMEM;
>> +     if (IS_ERR(chunk)) {
>> +             retval = PTR_ERR(chunk);
>>               goto out;
>>       }
>>
>> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>>       }
>>
>>       chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
>> -     if (!chunk)
>> +     if (IS_ERR(chunk)) {
>> +             chunk = NULL;
>>               goto out;
>> +     }
>>
>>       if (nums)
>>               for (i = 0; i < nums; i++)
>> --
>> 2.1.0
>>
>>

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Marcelo Ricardo Leitner]

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

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Marcelo Ricardo Leitner]

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

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

On Tue, Nov 14, 2017 at 3:54 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> 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.
right.

>
>> >>       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.
right.

let's move the check into sctp_send_reset_streams()

I believe this one fixes them both:
@@ -139,15 +139,31 @@ int sctp_send_reset_streams(struct sctp_association *asoc,

        str_nums = params->srs_number_streams;
        str_list = params->srs_stream_list;
-       if (out && str_nums)
-               for (i = 0; i < str_nums; i++)
-                       if (str_list[i] >= stream->outcnt)
-                               goto out;
+       if (str_nums) {
+               int param_len = 0;

-       if (in && str_nums)
-               for (i = 0; i < str_nums; i++)
-                       if (str_list[i] >= stream->incnt)
-                               goto out;
+               if (out) {
+                       for (i = 0; i < str_nums; i++)
+                               if (str_list[i] >= stream->outcnt)
+                                       goto out;
+
+                       param_len = str_nums * 2 +
+                                   sizeof(struct sctp_strreset_outreq);
+               }
+
+               if (in) {
+                       for (i = 0; i < str_nums; i++)
+                               if (str_list[i] >= stream->incnt)
+                                       goto out;
+
+                       param_len += str_nums * 2 +
+                                    sizeof(struct sctp_strreset_inreq);
+               }
+
+               if (param_len > SCTP_MAX_CHUNK_LEN -
+                               sizeof(struct sctp_reconf_chunk))
+                       goto out;
+       }


and int this fix,  it's good to do all checks only when str_nums !=0.

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

On Tue, Nov 14, 2017 at 3:54 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> 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.
right.

>
>> >>       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.
right.

let's move the check into sctp_send_reset_streams()

I believe this one fixes them both:
@@ -139,15 +139,31 @@ int sctp_send_reset_streams(struct sctp_association *asoc,

        str_nums = params->srs_number_streams;
        str_list = params->srs_stream_list;
-       if (out && str_nums)
-               for (i = 0; i < str_nums; i++)
-                       if (str_list[i] >= stream->outcnt)
-                               goto out;
+       if (str_nums) {
+               int param_len = 0;

-       if (in && str_nums)
-               for (i = 0; i < str_nums; i++)
-                       if (str_list[i] >= stream->incnt)
-                               goto out;
+               if (out) {
+                       for (i = 0; i < str_nums; i++)
+                               if (str_list[i] >= stream->outcnt)
+                                       goto out;
+
+                       param_len = str_nums * 2 +
+                                   sizeof(struct sctp_strreset_outreq);
+               }
+
+               if (in) {
+                       for (i = 0; i < str_nums; i++)
+                               if (str_list[i] >= stream->incnt)
+                                       goto out;
+
+                       param_len += str_nums * 2 +
+                                    sizeof(struct sctp_strreset_inreq);
+               }
+
+               if (param_len > SCTP_MAX_CHUNK_LEN -
+                               sizeof(struct sctp_reconf_chunk))
+                       goto out;
+       }


and int this fix,  it's good to do all checks only when str_nums !=0.

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Neil Horman]

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;
> >>       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 ?
> 
I'm ok with ENOMEM.  If someone tries to reconf the streams with huge values,
ENOMEM makes sense to me
Neil

> >
> > Neil
> >
> >>       retval = sctp_make_reconf(asoc, outlen + inlen);
> >>       if (!retval)
> >> -             return NULL;
> >> +             return ERR_PTR(-ENOMEM);
> >>
> >>       if (outlen) {
> >>               outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
> >> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> >> index fa8371f..51a25bf 100644
> >> --- a/net/sctp/stream.c
> >> +++ b/net/sctp/stream.c
> >> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
> >>
> >>       kfree(nstr_list);
> >>
> >> -     if (!chunk) {
> >> -             retval = -ENOMEM;
> >> +     if (IS_ERR(chunk)) {
> >> +             retval = PTR_ERR(chunk);
> >>               goto out;
> >>       }
> >>
> >> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
> >>       }
> >>
> >>       chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
> >> -     if (!chunk)
> >> +     if (IS_ERR(chunk)) {
> >> +             chunk = NULL;
> >>               goto out;
> >> +     }
> >>
> >>       if (nums)
> >>               for (i = 0; i < nums; i++)
> >> --
> >> 2.1.0
> >>
> >>
> 

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Neil Horman]

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;
> >>       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 ?
> 
I'm ok with ENOMEM.  If someone tries to reconf the streams with huge values,
ENOMEM makes sense to me
Neil

> >
> > Neil
> >
> >>       retval = sctp_make_reconf(asoc, outlen + inlen);
> >>       if (!retval)
> >> -             return NULL;
> >> +             return ERR_PTR(-ENOMEM);
> >>
> >>       if (outlen) {
> >>               outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
> >> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
> >> index fa8371f..51a25bf 100644
> >> --- a/net/sctp/stream.c
> >> +++ b/net/sctp/stream.c
> >> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
> >>
> >>       kfree(nstr_list);
> >>
> >> -     if (!chunk) {
> >> -             retval = -ENOMEM;
> >> +     if (IS_ERR(chunk)) {
> >> +             retval = PTR_ERR(chunk);
> >>               goto out;
> >>       }
> >>
> >> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
> >>       }
> >>
> >>       chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
> >> -     if (!chunk)
> >> +     if (IS_ERR(chunk)) {
> >> +             chunk = NULL;
> >>               goto out;
> >> +     }
> >>
> >>       if (nums)
> >>               for (i = 0; i < nums; i++)
> >> --
> >> 2.1.0
> >>
> >>
> 

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

On Tue, Nov 14, 2017 at 8:46 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 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;
>> >>       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 ?
>>
> I'm ok with ENOMEM.  If someone tries to reconf the streams with huge values,
> ENOMEM makes sense to me
okay, thanks.

I will move the check to sctp_send_reset_streams() to fix
both this one and the one Marcelo mentioned.

> Neil
>
>> >
>> > Neil
>> >
>> >>       retval = sctp_make_reconf(asoc, outlen + inlen);
>> >>       if (!retval)
>> >> -             return NULL;
>> >> +             return ERR_PTR(-ENOMEM);
>> >>
>> >>       if (outlen) {
>> >>               outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
>> >> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> >> index fa8371f..51a25bf 100644
>> >> --- a/net/sctp/stream.c
>> >> +++ b/net/sctp/stream.c
>> >> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>> >>
>> >>       kfree(nstr_list);
>> >>
>> >> -     if (!chunk) {
>> >> -             retval = -ENOMEM;
>> >> +     if (IS_ERR(chunk)) {
>> >> +             retval = PTR_ERR(chunk);
>> >>               goto out;
>> >>       }
>> >>
>> >> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>> >>       }
>> >>
>> >>       chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
>> >> -     if (!chunk)
>> >> +     if (IS_ERR(chunk)) {
>> >> +             chunk = NULL;
>> >>               goto out;
>> >> +     }
>> >>
>> >>       if (nums)
>> >>               for (i = 0; i < nums; i++)
>> >> --
>> >> 2.1.0
>> >>
>> >>
>>

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Xin Long]

On Tue, Nov 14, 2017 at 8:46 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> 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;
>> >>       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 ?
>>
> I'm ok with ENOMEM.  If someone tries to reconf the streams with huge values,
> ENOMEM makes sense to me
okay, thanks.

I will move the check to sctp_send_reset_streams() to fix
both this one and the one Marcelo mentioned.

> Neil
>
>> >
>> > Neil
>> >
>> >>       retval = sctp_make_reconf(asoc, outlen + inlen);
>> >>       if (!retval)
>> >> -             return NULL;
>> >> +             return ERR_PTR(-ENOMEM);
>> >>
>> >>       if (outlen) {
>> >>               outreq.param_hdr.type = SCTP_PARAM_RESET_OUT_REQUEST;
>> >> diff --git a/net/sctp/stream.c b/net/sctp/stream.c
>> >> index fa8371f..51a25bf 100644
>> >> --- a/net/sctp/stream.c
>> >> +++ b/net/sctp/stream.c
>> >> @@ -162,8 +162,8 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
>> >>
>> >>       kfree(nstr_list);
>> >>
>> >> -     if (!chunk) {
>> >> -             retval = -ENOMEM;
>> >> +     if (IS_ERR(chunk)) {
>> >> +             retval = PTR_ERR(chunk);
>> >>               goto out;
>> >>       }
>> >>
>> >> @@ -482,8 +482,10 @@ struct sctp_chunk *sctp_process_strreset_inreq(
>> >>       }
>> >>
>> >>       chunk = sctp_make_strreset_req(asoc, nums, str_p, 1, 0);
>> >> -     if (!chunk)
>> >> +     if (IS_ERR(chunk)) {
>> >> +             chunk = NULL;
>> >>               goto out;
>> >> +     }
>> >>
>> >>       if (nums)
>> >>               for (i = 0; i < nums; i++)
>> >> --
>> >> 2.1.0
>> >>
>> >>
>>

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Marcelo Ricardo Leitner]

On Tue, Nov 14, 2017 at 04:27:41PM +0800, Xin Long wrote:
> On Tue, Nov 14, 2017 at 3:54 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > 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.
> right.
> 
> >
> >> >>       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.
> right.
> 
> let's move the check into sctp_send_reset_streams()
> 
> I believe this one fixes them both:
> @@ -139,15 +139,31 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
> 
>         str_nums = params->srs_number_streams;
>         str_list = params->srs_stream_list;
> -       if (out && str_nums)
> -               for (i = 0; i < str_nums; i++)
> -                       if (str_list[i] >= stream->outcnt)
> -                               goto out;
> +       if (str_nums) {
> +               int param_len = 0;
> 
> -       if (in && str_nums)
> -               for (i = 0; i < str_nums; i++)
> -                       if (str_list[i] >= stream->incnt)
> -                               goto out;
> +               if (out) {
> +                       for (i = 0; i < str_nums; i++)
> +                               if (str_list[i] >= stream->outcnt)
> +                                       goto out;
> +
> +                       param_len = str_nums * 2 +
                                 sizeof(__u16) --^

> +                                   sizeof(struct sctp_strreset_outreq);
> +               }
> +
> +               if (in) {
> +                       for (i = 0; i < str_nums; i++)
> +                               if (str_list[i] >= stream->incnt)
> +                                       goto out;
> +
> +                       param_len += str_nums * 2 +
                                  sizeof(__u16) --^

> +                                    sizeof(struct sctp_strreset_inreq);
> +               }
> +
> +               if (param_len > SCTP_MAX_CHUNK_LEN -
> +                               sizeof(struct sctp_reconf_chunk))
> +                       goto out;
> +       }
> 
> 
> and int this fix,  it's good to do all checks only when str_nums !=0.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH net] sctp: check stream reset info len before making reconf chunk

[Marcelo Ricardo Leitner]

On Tue, Nov 14, 2017 at 04:27:41PM +0800, Xin Long wrote:
> On Tue, Nov 14, 2017 at 3:54 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > 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.
> right.
> 
> >
> >> >>       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.
> right.
> 
> let's move the check into sctp_send_reset_streams()
> 
> I believe this one fixes them both:
> @@ -139,15 +139,31 @@ int sctp_send_reset_streams(struct sctp_association *asoc,
> 
>         str_nums = params->srs_number_streams;
>         str_list = params->srs_stream_list;
> -       if (out && str_nums)
> -               for (i = 0; i < str_nums; i++)
> -                       if (str_list[i] >= stream->outcnt)
> -                               goto out;
> +       if (str_nums) {
> +               int param_len = 0;
> 
> -       if (in && str_nums)
> -               for (i = 0; i < str_nums; i++)
> -                       if (str_list[i] >= stream->incnt)
> -                               goto out;
> +               if (out) {
> +                       for (i = 0; i < str_nums; i++)
> +                               if (str_list[i] >= stream->outcnt)
> +                                       goto out;
> +
> +                       param_len = str_nums * 2 +
                                 sizeof(__u16) --^

> +                                   sizeof(struct sctp_strreset_outreq);
> +               }
> +
> +               if (in) {
> +                       for (i = 0; i < str_nums; i++)
> +                               if (str_list[i] >= stream->incnt)
> +                                       goto out;
> +
> +                       param_len += str_nums * 2 +
                                  sizeof(__u16) --^

> +                                    sizeof(struct sctp_strreset_inreq);
> +               }
> +
> +               if (param_len > SCTP_MAX_CHUNK_LEN -
> +                               sizeof(struct sctp_reconf_chunk))
> +                       goto out;
> +       }
> 
> 
> and int this fix,  it's good to do all checks only when str_nums !=0.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>