Skip to contentSkip to conversation

sctp: fix reporting of unknown parameters

RFC PATCH Applied Jiri Bohac 4 participants 7 messages Feb 17, 2011 – Feb 22, 2011

Subsystems (3)
Networking Network drivers SCTP protocol
lore
Share & tools
lore

Jiri Bohac

author
#0 lore raw

RFC PATCH 1/1 sctp: fix reporting of unknown parameters

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 <[email protected]>
net/sctp/sm_make_chunk.c +5 -5
index 2cc46f0..b23428f 100644
@@ -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
Signature · 2 lines
Jiri Bohac <[email protected]>
SUSE Labs, SUSE CZ
#1 lore raw

(cc vlad Yasevich)

Jiri Bohac wrote, at 02/18/2011 07:12 AM:

Quoted 10 lines

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 <[email protected]>

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

Quoted 25 lines
net/sctp/sm_make_chunk.c +5 -5
index 2cc46f0..b23428f 100644
@@ -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
Signature · 3 lines
Best Regards
-----
Shan Wei
author
#2 lore raw
Quoted 11 lines · Shan Wei

Jiri Bohac wrote, at 02/18/2011 07:12 AM:

Quoted 8 lines

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 <[email protected]>

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.

Quoted 3 lines

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.

Signature · 2 lines
Jiri Bohac <[email protected]>
SUSE Labs, SUSE CZ
Acked-by
#3 lore raw
Quoted 33 lines · Jiri Bohac
Quoted 11 lines · Shan Wei

Jiri Bohac wrote, at 02/18/2011 07:12 AM:

Quoted 8 lines

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 <[email protected]>

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.

Quoted 7 lines
Quoted 3 lines

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 <[email protected]>

-vlad

#4 lore raw
In reply to Jiri Bohac

Hi vald:

Vladislav Yasevich wrote, at 02/18/2011 10:43 PM:

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 8 lines
Acked-by Vlad Yasevich <[email protected]>

-vlad

Signature · 3 lines
Best Regards
-----
Shan Wei
maintainer Applied
#5 lore raw
In reply to Jiri Bohac

From: Vladislav Yasevich <[email protected]>
Date: Fri, 18 Feb 2011 09:43:30 -0500

Quoted 45 lines · Jiri Bohac
Quoted 33 lines · Shan Wei
Quoted 11 lines

Jiri Bohac wrote, at 02/18/2011 07:12 AM:

Quoted 8 lines

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 <[email protected]>

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.

Quoted 7 lines
Quoted 3 lines

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

Quoted 8 lines

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 <[email protected]>

Applied, thanks everyone.

#6 lore raw
In reply to Jiri Bohac
Quoted 30 lines · Shan Wei

Hi vald:

Vladislav Yasevich wrote, at 02/18/2011 10:43 PM:

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 5 lines

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

Quoted 10 lines
Quoted 8 lines
Acked-by Vlad Yasevich <[email protected]>

-vlad