[PATCH][RFC] sctp: fix reporting of unknown parameters

[PATCH][RFC] sctp: fix reporting of unknown parameters

[Jiri Bohac]

Hi,

commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
handling of unknown parameters. sctp_init_cause_fixed() can now
return -ENOSPC if there is not enough tailroom in the error
chunk skb. When this happens, the error header is not appended to
the error chunk. In that case, the payload of the unknown parameter
should not be appended either.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>


diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 2cc46f0..b23428f 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2029,11 +2029,11 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 			*errp = sctp_make_op_error_fixed(asoc, chunk);
 
 		if (*errp) {
-			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
-					WORD_ROUND(ntohs(param.p->length)));
-			sctp_addto_chunk_fixed(*errp,
-					WORD_ROUND(ntohs(param.p->length)),
-					param.v);
+			if (!sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
+					WORD_ROUND(ntohs(param.p->length))))
+				sctp_addto_chunk_fixed(*errp,
+						WORD_ROUND(ntohs(param.p->length)),
+						param.v);
 		} else {
 			/* If there is no memory for generating the ERROR
 			 * report as specified, an ABORT will be triggered


-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

[PATCH][RFC] sctp: fix reporting of unknown parameters

[Jiri Bohac]

Hi,

commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
handling of unknown parameters. sctp_init_cause_fixed() can now
return -ENOSPC if there is not enough tailroom in the error
chunk skb. When this happens, the error header is not appended to
the error chunk. In that case, the payload of the unknown parameter
should not be appended either.

Signed-off-by: Jiri Bohac <jbohac@suse.cz>


diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index 2cc46f0..b23428f 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -2029,11 +2029,11 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
 			*errp = sctp_make_op_error_fixed(asoc, chunk);
 
 		if (*errp) {
-			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
-					WORD_ROUND(ntohs(param.p->length)));
-			sctp_addto_chunk_fixed(*errp,
-					WORD_ROUND(ntohs(param.p->length)),
-					param.v);
+			if (!sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
+					WORD_ROUND(ntohs(param.p->length))))
+				sctp_addto_chunk_fixed(*errp,
+						WORD_ROUND(ntohs(param.p->length)),
+						param.v);
 		} else {
 			/* If there is no memory for generating the ERROR
 			 * report as specified, an ABORT will be triggered


-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Shan Wei]

(cc vlad Yasevich)

Jiri Bohac wrote, at 02/18/2011 07:12 AM:
> Hi,
> 
> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
> handling of unknown parameters. sctp_init_cause_fixed() can now
> return -ENOSPC if there is not enough tailroom in the error
> chunk skb. When this happens, the error header is not appended to
> the error chunk. In that case, the payload of the unknown parameter
> should not be appended either.
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>

For this case, there is no more tailroom in skb, originally
we send incomplete INIT-ACK chunk. With your patch, this chunk also 
can't info the sender of INIT Chunk which parameter is not unrecognized.
So maybe this handle is not perfect. 

So, for this case, i think send ABORT is more appropriate.
Just take same handle with SCTP_IERROR_NOMEM

> 
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 2cc46f0..b23428f 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2029,11 +2029,11 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
>  			*errp = sctp_make_op_error_fixed(asoc, chunk);
>  
>  		if (*errp) {
> -			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> -					WORD_ROUND(ntohs(param.p->length)));
> -			sctp_addto_chunk_fixed(*errp,
> -					WORD_ROUND(ntohs(param.p->length)),
> -					param.v);
> +			if (!sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> +					WORD_ROUND(ntohs(param.p->length))))
> +				sctp_addto_chunk_fixed(*errp,
> +						WORD_ROUND(ntohs(param.p->length)),
> +						param.v);
>  		} else {
>  			/* If there is no memory for generating the ERROR
>  			 * report as specified, an ABORT will be triggered
> 
> 


-- 

Best Regards
-----
Shan Wei

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Shan Wei]

(cc vlad Yasevich)

Jiri Bohac wrote, at 02/18/2011 07:12 AM:
> Hi,
> 
> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
> handling of unknown parameters. sctp_init_cause_fixed() can now
> return -ENOSPC if there is not enough tailroom in the error
> chunk skb. When this happens, the error header is not appended to
> the error chunk. In that case, the payload of the unknown parameter
> should not be appended either.
> 
> Signed-off-by: Jiri Bohac <jbohac@suse.cz>

For this case, there is no more tailroom in skb, originally
we send incomplete INIT-ACK chunk. With your patch, this chunk also 
can't info the sender of INIT Chunk which parameter is not unrecognized.
So maybe this handle is not perfect. 

So, for this case, i think send ABORT is more appropriate.
Just take same handle with SCTP_IERROR_NOMEM

> 
> 
> diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
> index 2cc46f0..b23428f 100644
> --- a/net/sctp/sm_make_chunk.c
> +++ b/net/sctp/sm_make_chunk.c
> @@ -2029,11 +2029,11 @@ static sctp_ierror_t sctp_process_unk_param(const struct sctp_association *asoc,
>  			*errp = sctp_make_op_error_fixed(asoc, chunk);
>  
>  		if (*errp) {
> -			sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> -					WORD_ROUND(ntohs(param.p->length)));
> -			sctp_addto_chunk_fixed(*errp,
> -					WORD_ROUND(ntohs(param.p->length)),
> -					param.v);
> +			if (!sctp_init_cause_fixed(*errp, SCTP_ERROR_UNKNOWN_PARAM,
> +					WORD_ROUND(ntohs(param.p->length))))
> +				sctp_addto_chunk_fixed(*errp,
> +						WORD_ROUND(ntohs(param.p->length)),
> +						param.v);
>  		} else {
>  			/* If there is no memory for generating the ERROR
>  			 * report as specified, an ABORT will be triggered
> 
> 


-- 

Best Regards
-----
Shan Wei

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Jiri Bohac]

On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
> > commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
> > handling of unknown parameters. sctp_init_cause_fixed() can now
> > return -ENOSPC if there is not enough tailroom in the error
> > chunk skb. When this happens, the error header is not appended to
> > the error chunk. In that case, the payload of the unknown parameter
> > should not be appended either.
> > 
> > Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> 
> For this case, there is no more tailroom in skb

not always true:

both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
parameter.

sctp_init_cause_fixed() does:
	len = sizeof(sctp_errhdr_t) + paylen;
        if (skb_tailroom(chunk->skb) < len)
	                return -ENOSPC;

if paylen is only slightly smaller than the tailroom (by less
than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
the header will not be appended to the error chunk.

But later sctp_addto_chunk_fixed() does this check:
	if (skb_tailroom(chunk->skb) >= len)
which will pass and the unknown parameter value will be appended
to the error chunk without the error header.

> we send incomplete INIT-ACK chunk. With your patch, this chunk also 
> can't info the sender of INIT Chunk which parameter is not unrecognized.
> So maybe this handle is not perfect. 

The logic of not reporting unknown parameters for which we don't
have space in the pre-allocated buffer remains unchanged.

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Jiri Bohac]

On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
> > commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
> > handling of unknown parameters. sctp_init_cause_fixed() can now
> > return -ENOSPC if there is not enough tailroom in the error
> > chunk skb. When this happens, the error header is not appended to
> > the error chunk. In that case, the payload of the unknown parameter
> > should not be appended either.
> > 
> > Signed-off-by: Jiri Bohac <jbohac@suse.cz>
> 
> For this case, there is no more tailroom in skb

not always true:

both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
parameter.

sctp_init_cause_fixed() does:
	len = sizeof(sctp_errhdr_t) + paylen;
        if (skb_tailroom(chunk->skb) < len)
	                return -ENOSPC;

if paylen is only slightly smaller than the tailroom (by less
than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
the header will not be appended to the error chunk.

But later sctp_addto_chunk_fixed() does this check:
	if (skb_tailroom(chunk->skb) >= len)
which will pass and the unknown parameter value will be appended
to the error chunk without the error header.

> we send incomplete INIT-ACK chunk. With your patch, this chunk also 
> can't info the sender of INIT Chunk which parameter is not unrecognized.
> So maybe this handle is not perfect. 

The logic of not reporting unknown parameters for which we don't
have space in the pre-allocated buffer remains unchanged.

-- 
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Vladislav Yasevich]

On 02/18/2011 07:01 AM, Jiri Bohac wrote:
> On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
>> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
>>> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
>>> handling of unknown parameters. sctp_init_cause_fixed() can now
>>> return -ENOSPC if there is not enough tailroom in the error
>>> chunk skb. When this happens, the error header is not appended to
>>> the error chunk. In that case, the payload of the unknown parameter
>>> should not be appended either.
>>>
>>> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>>
>> For this case, there is no more tailroom in skb
> 
> not always true:
> 
> both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
> passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
> parameter.
> 
> sctp_init_cause_fixed() does:
> 	len = sizeof(sctp_errhdr_t) + paylen;
>         if (skb_tailroom(chunk->skb) < len)
> 	                return -ENOSPC;
> 
> if paylen is only slightly smaller than the tailroom (by less
> than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
> the header will not be appended to the error chunk.
> 
> But later sctp_addto_chunk_fixed() does this check:
> 	if (skb_tailroom(chunk->skb) >= len)
> which will pass and the unknown parameter value will be appended
> to the error chunk without the error header.
> 
>> we send incomplete INIT-ACK chunk. With your patch, this chunk also 
>> can't info the sender of INIT Chunk which parameter is not unrecognized.
>> So maybe this handle is not perfect. 
> 
> The logic of not reporting unknown parameters for which we don't
> have space in the pre-allocated buffer remains unchanged.
> 


Hi Jiri

I agree.  Good catch.  If we can not add the error header to that packet, then we definitely
should not be adding the error payload either.

We also don't want to abort as Shan Wei suggested.  It is allowed, when exceeding the MTU to
drop errors that would not otherwise fit into the packet.  It just may so happen that we
can report subsequent unknown parameters, but not this one in particular.

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Vladislav Yasevich]

On 02/18/2011 07:01 AM, Jiri Bohac wrote:
> On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
>> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
>>> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
>>> handling of unknown parameters. sctp_init_cause_fixed() can now
>>> return -ENOSPC if there is not enough tailroom in the error
>>> chunk skb. When this happens, the error header is not appended to
>>> the error chunk. In that case, the payload of the unknown parameter
>>> should not be appended either.
>>>
>>> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>>
>> For this case, there is no more tailroom in skb
> 
> not always true:
> 
> both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
> passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
> parameter.
> 
> sctp_init_cause_fixed() does:
> 	len = sizeof(sctp_errhdr_t) + paylen;
>         if (skb_tailroom(chunk->skb) < len)
> 	                return -ENOSPC;
> 
> if paylen is only slightly smaller than the tailroom (by less
> than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
> the header will not be appended to the error chunk.
> 
> But later sctp_addto_chunk_fixed() does this check:
> 	if (skb_tailroom(chunk->skb) >= len)
> which will pass and the unknown parameter value will be appended
> to the error chunk without the error header.
> 
>> we send incomplete INIT-ACK chunk. With your patch, this chunk also 
>> can't info the sender of INIT Chunk which parameter is not unrecognized.
>> So maybe this handle is not perfect. 
> 
> The logic of not reporting unknown parameters for which we don't
> have space in the pre-allocated buffer remains unchanged.
> 


Hi Jiri

I agree.  Good catch.  If we can not add the error header to that packet, then we definitely
should not be adding the error payload either.

We also don't want to abort as Shan Wei suggested.  It is allowed, when exceeding the MTU to
drop errors that would not otherwise fit into the packet.  It just may so happen that we
can report subsequent unknown parameters, but not this one in particular.

Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

-vlad

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Shan Wei]

Hi vald:

Vladislav Yasevich wrote, at 02/18/2011 10:43 PM:
> Hi Jiri
> 
> I agree.  Good catch.  If we can not add the error header to that packet, then we definitely
> should not be adding the error payload either.

I also agree this part.
 
> We also don't want to abort as Shan Wei suggested.  It is allowed, when exceeding the MTU to
> drop errors that would not otherwise fit into the packet.  It just may so happen that we
> can report subsequent unknown parameters, but not this one in particular.

RFC said that about Parameter Types' encode:
3.2.1.  Optional/Variable-Length Parameter Format
      11 -  Skip this parameter and continue processing but report the
            unrecognized parameter in an 'Unrecognized Parameter', as
            described in Section 3.2.2.

and

3.2.2.  Reporting of Unrecognized Parameters

   If the receiver of an INIT chunk detects unrecognized parameters and
   *has to* report them according to Section 3.2.1, it MUST put the
   'Unrecognized Parameter' parameter(s) in the INIT ACK chunk sent in
   response to the INIT chunk.

So, For unrecognized parameter can't fit into the packet, so droping the parameter.
This may not be consistent with the RFC.

If i have missed something, please tell me. Thanks.

> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
> 
> -vlad
> 
> 
> 
> 


-- 

Best Regards
-----
Shan Wei

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[David Miller]

From: Vladislav Yasevich <vladislav.yasevich@hp.com>
Date: Fri, 18 Feb 2011 09:43:30 -0500

> On 02/18/2011 07:01 AM, Jiri Bohac wrote:
>> On Fri, Feb 18, 2011 at 07:26:18PM +0800, Shan Wei wrote:
>>> Jiri Bohac wrote, at 02/18/2011 07:12 AM:
>>>> commit 5fa782c2f5ef6c2e4f04d3e228412c9b4a4c8809 re-worked the 
>>>> handling of unknown parameters. sctp_init_cause_fixed() can now
>>>> return -ENOSPC if there is not enough tailroom in the error
>>>> chunk skb. When this happens, the error header is not appended to
>>>> the error chunk. In that case, the payload of the unknown parameter
>>>> should not be appended either.
>>>>
>>>> Signed-off-by: Jiri Bohac <jbohac@suse.cz>
>>>
>>> For this case, there is no more tailroom in skb
>> 
>> not always true:
>> 
>> both sctp_init_cause_fixed() and sctp_addto_chunk_fixed() get
>> passesd WORD_ROUND(ntohs(param.p->length) as the paylen/len
>> parameter.
>> 
>> sctp_init_cause_fixed() does:
>> 	len = sizeof(sctp_errhdr_t) + paylen;
>>         if (skb_tailroom(chunk->skb) < len)
>> 	                return -ENOSPC;
>> 
>> if paylen is only slightly smaller than the tailroom (by less
>> than sizeof(sctp_errhdr_t) bytes), -ENOSPEC will be returned and
>> the header will not be appended to the error chunk.
>> 
>> But later sctp_addto_chunk_fixed() does this check:
>> 	if (skb_tailroom(chunk->skb) >= len)
>> which will pass and the unknown parameter value will be appended
>> to the error chunk without the error header.
>> 
>>> we send incomplete INIT-ACK chunk. With your patch, this chunk also 
>>> can't info the sender of INIT Chunk which parameter is not unrecognized.
>>> So maybe this handle is not perfect. 
>> 
>> The logic of not reporting unknown parameters for which we don't
>> have space in the pre-allocated buffer remains unchanged.
>> 
> 
> 
> Hi Jiri
> 
> I agree.  Good catch.  If we can not add the error header to that packet, then we definitely
> should not be adding the error payload either.
> 
> We also don't want to abort as Shan Wei suggested.  It is allowed, when exceeding the MTU to
> drop errors that would not otherwise fit into the packet.  It just may so happen that we
> can report subsequent unknown parameters, but not this one in particular.
> 
> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>

Applied, thanks everyone.

Re: [PATCH][RFC] sctp: fix reporting of unknown parameters

[Vladislav Yasevich]

On 02/19/2011 10:07 PM, Shan Wei wrote:
> 
> Hi vald:
> 
> Vladislav Yasevich wrote, at 02/18/2011 10:43 PM:
>> Hi Jiri
>>
>> I agree.  Good catch.  If we can not add the error header to that packet, then we definitely
>> should not be adding the error payload either.
> 
> I also agree this part.
>  
>> We also don't want to abort as Shan Wei suggested.  It is allowed, when exceeding the MTU to
>> drop errors that would not otherwise fit into the packet.  It just may so happen that we
>> can report subsequent unknown parameters, but not this one in particular.
> 
> RFC said that about Parameter Types' encode:
> 3.2.1.  Optional/Variable-Length Parameter Format
>       11 -  Skip this parameter and continue processing but report the
>             unrecognized parameter in an 'Unrecognized Parameter', as
>             described in Section 3.2.2.
> 
> and
> 
> 3.2.2.  Reporting of Unrecognized Parameters
> 
>    If the receiver of an INIT chunk detects unrecognized parameters and
>    *has to* report them according to Section 3.2.1, it MUST put the
>    'Unrecognized Parameter' parameter(s) in the INIT ACK chunk sent in
>    response to the INIT chunk.
> 
> So, For unrecognized parameter can't fit into the packet, so droping the parameter.
> This may not be consistent with the RFC.
> 
> If i have missed something, please tell me. Thanks.
> 

See Secton 11.4.  Specifically:

   An SCTP implementation that receives an INIT that would require a
   large packet in response, due to the inclusion of multiple ERROR
   parameters, MAY (at its discretion) elect to omit some or all of the
   ERROR parameters to reduce the size of the INIT ACK.  Due to a
   combination of the size of the COOKIE parameter and the number of
   addresses a receiver of an INIT may be indicating to a peer, it is
   always possible that the INIT ACK will be larger than the original
   INIT.  An SCTP implementation SHOULD attempt to make the INIT ACK as
   small as possible to reduce the possibility of byte amplification
   attacks.


-vlad
>>
>> Acked-by: Vlad Yasevich <vladislav.yasevich@hp.com>
>>
>> -vlad
>>
>>
>>
>>
> 
>