[bug+patch] SCTP and chunk types over 0x1f

[bug+patch] SCTP and chunk types over 0x1f

[Stephane Chazelas]

Hi guys,

I'm sending this to the list as the bugzilla site seems to be
down at least at the moment.

There seems to be several bugs in the iptable and the kernel
code that prevent the filtering on SCTP chunks of type above 31
to work.

Below is a patch for iptables, the corresponding header files
in the kernel would have to be updated the same way.

The problems:
- the values for the ASCONF and ASCONF_ACK types were incorrect
  (30, 31 instead of 0x80 or 0xC0), I've also added the FTSN
  chunk (RFC 3758)

- the chunkmap, which is a bitmap of which chunk is selected is
  defined as an array of 256 / 4 u32s, that is 256 * 8 bits,
  256 / 32 is enough (256 bits).

- the macros like SCTP_CHUNKMAP_SET_ALL... use
  ELEMCOUNT/ARRAY_SIZE to loop through the u32s of the chunkmap.
  But those macros are sometimes called with a u_int32_t*
  instead of a u_int32_t[8], so that it loops only on the first
  u32.

- bug in print_chunk(), see below.

both the kernel and iptables need to be updated, at least to
take into account the new size of the chunkmap. That patch
doesn't try to be smart wrt to compatibility. It would be nice
to be able to specify chunk using their numerical value (to take
into account future SCTP extensions).

Best regards,
Stephane


Index: include/linux/netfilter_ipv4/ipt_sctp.h
===================================================================
--- include/linux/netfilter_ipv4/ipt_sctp.h	(revision 6882)
+++ include/linux/netfilter_ipv4/ipt_sctp.h	(working copy)
@@ -22,7 +22,7 @@ struct ipt_sctp_info {
 	u_int16_t dpts[2];  /* Min, Max */
 	u_int16_t spts[2];  /* Min, Max */
 
-	u_int32_t chunkmap[256 / sizeof (u_int32_t)];  /* Bit mask of chunks to be matched according to RFC 2960 */
+	u_int32_t chunkmap[256 / 32];  /* Bit mask of chunks to be matched according to RFC 2960 */
 
 #define SCTP_CHUNK_MATCH_ANY   0x01  /* Match if any of the chunk types are present */
 #define SCTP_CHUNK_MATCH_ALL   0x02  /* Match if all of the chunk types are present */
@@ -36,44 +36,42 @@ struct ipt_sctp_info {
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
-
 #define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
 	do { 						\
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+		chunkmap[type / 32] |= 	\
+			1 << (type % 32);	\
 	} while (0)
 
 #define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
 	do {							\
-		chunkmap[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+		chunkmap[type / 32] &= 		\
+			~(1 << (type % 32));	\
 	} while (0)
 
 #define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
 ({								\
-	(chunkmap[type / bytes (u_int32_t)] & 			\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+	(chunkmap[type / 32] & 			\
+		(1 << (type % 32))) ? 1: 0;	\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) 				\
 	do {							\
 		int i; 						\
-		for (i = 0; i < ELEMCOUNT(chunkmap); i++)	\
+		for (i = 0; i < (256 / 32); i++)	\
 			chunkmap[i] = 0;			\
 	} while (0)
 
 #define SCTP_CHUNKMAP_SET_ALL(chunkmap) 			\
 	do {							\
 		int i; 						\
-		for (i = 0; i < ELEMCOUNT(chunkmap); i++) 	\
+		for (i = 0; i < (256 / 32); i++) 	\
 			chunkmap[i] = ~0;			\
 	} while (0)
 
 #define SCTP_CHUNKMAP_COPY(destmap, srcmap) 			\
 	do {							\
 		int i; 						\
-		for (i = 0; i < ELEMCOUNT(chunkmap); i++) 	\
+		for (i = 0; i < (256 / 32); i++) 	\
 			destmap[i] = srcmap[i];			\
 	} while (0)
 
@@ -81,7 +79,7 @@ struct ipt_sctp_info {
 ({							\
 	int i; 						\
 	int flag = 1;					\
-	for (i = 0; i < ELEMCOUNT(chunkmap); i++) {	\
+	for (i = 0; i < (256 / 32); i++) {	\
 		if (chunkmap[i]) {			\
 			flag = 0;			\
 			break;				\
@@ -94,7 +92,7 @@ struct ipt_sctp_info {
 ({							\
 	int i; 						\
 	int flag = 1;					\
-	for (i = 0; i < ELEMCOUNT(chunkmap); i++) {	\
+	for (i = 0; i < (256 / 32); i++) {	\
 		if (chunkmap[i] != ~0) {		\
 			flag = 0;			\
 				break;			\
Index: extensions/libipt_sctp.c
===================================================================
--- extensions/libipt_sctp.c	(revision 6882)
+++ extensions/libipt_sctp.c	(working copy)
@@ -66,7 +66,7 @@ static void help(void)
 " --dport ...\n" 
 " --chunk-types [!] (all|any|none) (chunktype[:flags])+	match if all, any or none of\n"
 "						        chunktypes are present\n"
-"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE ASCONF ASCONF_ACK ALL NONE\n",
+"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE FTSN ASCONF ASCONF_ACK ALL NONE\n",
 	IPTABLES_VERSION);
 }
 
@@ -128,8 +128,9 @@ static struct sctp_chunk_names sctp_chun
     { .name = "ECN_ECNE",	.chunk_type = 12,  .valid_flags = "--------"},
     { .name = "ECN_CWR",	.chunk_type = 13,  .valid_flags = "--------"},
     { .name = "SHUTDOWN_COMPLETE", .chunk_type = 14,  .valid_flags = "-------T"},
-    { .name = "ASCONF",		.chunk_type = 31,  .valid_flags = "--------"},
-    { .name = "ASCONF_ACK",	.chunk_type = 30,  .valid_flags = "--------"},
+    { .name = "FTSN",		.chunk_type = 0xC0,  .valid_flags = "--------"},
+    { .name = "ASCONF",		.chunk_type = 0xC1,  .valid_flags = "--------"},
+    { .name = "ASCONF_ACK",	.chunk_type = 0x80,  .valid_flags = "--------"},
 };
 
 static void
@@ -402,14 +403,14 @@ print_chunk(u_int32_t chunknum, int nume
 
 		for (i = 0; i < ELEMCOUNT(sctp_chunk_names); i++) {
 			if (sctp_chunk_names[i].chunk_type == chunknum)
-				printf("%s", sctp_chunk_names[chunknum].name);
+				printf("%s", sctp_chunk_names[i].name);
 		}
 	}
 }
 
 static void
 print_chunks(u_int32_t chunk_match_type, 
-	     const u_int32_t *chunkmap, 
+	     const u_int32_t *chunkmap,
 	     const struct ipt_sctp_flag_info *flag_info,
 	     int flag_count,
 	     int numeric)
Index: extensions/libip6t_sctp.c
===================================================================
--- extensions/libip6t_sctp.c	(revision 6882)
+++ extensions/libip6t_sctp.c	(working copy)
@@ -66,7 +66,7 @@ static void help(void)
 " --dport ...\n" 
 " --chunk-types [!] (all|any|none) (chunktype[:flags])+	match if all, any or none of\n"
 "						        chunktypes are present\n"
-"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE ASCONF ASCONF_ACK ALL NONE\n",
+"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE FTSN ASCONF ASCONF_ACK ALL NONE\n",
 	IPTABLES_VERSION);
 }
 
@@ -128,8 +128,9 @@ static struct sctp_chunk_names sctp_chun
     { .name = "ECN_ECNE",	.chunk_type = 12,  .valid_flags = "--------"},
     { .name = "ECN_CWR",	.chunk_type = 13,  .valid_flags = "--------"},
     { .name = "SHUTDOWN_COMPLETE", .chunk_type = 14,  .valid_flags = "-------T"},
-    { .name = "ASCONF",		.chunk_type = 31,  .valid_flags = "--------"},
-    { .name = "ASCONF_ACK",	.chunk_type = 30,  .valid_flags = "--------"},
+    { .name = "FTSN",		.chunk_type = 0xC0,  .valid_flags = "--------"},
+    { .name = "ASCONF",		.chunk_type = 0xC1,  .valid_flags = "--------"},
+    { .name = "ASCONF_ACK",	.chunk_type = 0x80,  .valid_flags = "--------"},
 };
 
 static void
@@ -402,7 +403,7 @@ print_chunk(u_int32_t chunknum, int nume
 
 		for (i = 0; i < ELEMCOUNT(sctp_chunk_names); i++) {
 			if (sctp_chunk_names[i].chunk_type == chunknum)
-				printf("%s", sctp_chunk_names[chunknum].name);
+				printf("%s", sctp_chunk_names[i].name);
 		}
 	}
 }

Re: [bug+patch] SCTP and chunk types over 0x1f

[Patrick McHardy]

Stephane Chazelas wrote:
> There seems to be several bugs in the iptable and the kernel
> code that prevent the filtering on SCTP chunks of type above 31
> to work.
> 
> Below is a patch for iptables, the corresponding header files
> in the kernel would have to be updated the same way.
> 
> The problems:
> - the values for the ASCONF and ASCONF_ACK types were incorrect
>   (30, 31 instead of 0x80 or 0xC0), I've also added the FTSN
>   chunk (RFC 3758)
> 
> - the chunkmap, which is a bitmap of which chunk is selected is
>   defined as an array of 256 / 4 u32s, that is 256 * 8 bits,
>   256 / 32 is enough (256 bits).


We can't change that since it breaks userspace compatibility.

> - the macros like SCTP_CHUNKMAP_SET_ALL... use
>   ELEMCOUNT/ARRAY_SIZE to loop through the u32s of the chunkmap.
>   But those macros are sometimes called with a u_int32_t*
>   instead of a u_int32_t[8], so that it loops only on the first
>   u32.

Nice catch.

> - bug in print_chunk(), see below.
> 
> both the kernel and iptables need to be updated, at least to
> take into account the new size of the chunkmap. That patch
> doesn't try to be smart wrt to compatibility. It would be nice
> to be able to specify chunk using their numerical value (to take
> into account future SCTP extensions).

Unfortunately this missed the 1.3.8 release since I didn't
notice it before.

Could you please resend without the chunkmap changes? Thanks.

Re: [bug+patch] SCTP and chunk types over 0x1f

[Stephane Chazelas]

On Mon, Jun 25, 2007 at 03:40:27PM +0200, Patrick McHardy wrote:
> Stephane Chazelas wrote:
> > There seems to be several bugs in the iptable and the kernel
> > code that prevent the filtering on SCTP chunks of type above 31
> > to work.
> > 
> > Below is a patch for iptables, the corresponding header files
> > in the kernel would have to be updated the same way.
> > 
> > The problems:
> > - the values for the ASCONF and ASCONF_ACK types were incorrect
> >   (30, 31 instead of 0x80 or 0xC0), I've also added the FTSN
> >   chunk (RFC 3758)
> > 
> > - the chunkmap, which is a bitmap of which chunk is selected is
> >   defined as an array of 256 / 4 u32s, that is 256 * 8 bits,
> >   256 / 32 is enough (256 bits).
> 
> 
> We can't change that since it breaks userspace compatibility.

Hi Patrick,

I see. But that doesn't look very tidy. Maybe some transitional
code with a warning in both the kernel and iptables?

[...]
> Could you please resend without the chunkmap changes? Thanks.

That's just a matter of removing the first change:

Index: include/linux/netfilter_ipv4/ipt_sctp.h
===================================================================
--- include/linux/netfilter_ipv4/ipt_sctp.h	(revision 6882)
+++ include/linux/netfilter_ipv4/ipt_sctp.h	(working copy)
@@ -36,44 +36,42 @@ struct ipt_sctp_info {
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
-
 #define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
 	do { 						\
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+		chunkmap[type / 32] |= 	\
+			1 << (type % 32);	\
 	} while (0)
 
 #define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
 	do {							\
-		chunkmap[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+		chunkmap[type / 32] &= 		\
+			~(1 << (type % 32));	\
 	} while (0)
 
 #define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
 ({								\
-	(chunkmap[type / bytes (u_int32_t)] & 			\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+	(chunkmap[type / 32] & 			\
+		(1 << (type % 32))) ? 1: 0;	\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) 				\
 	do {							\
 		int i; 						\
-		for (i = 0; i < ELEMCOUNT(chunkmap); i++)	\
+		for (i = 0; i < (256 / 32); i++)	\
 			chunkmap[i] = 0;			\
 	} while (0)
 
 #define SCTP_CHUNKMAP_SET_ALL(chunkmap) 			\
 	do {							\
 		int i; 						\
-		for (i = 0; i < ELEMCOUNT(chunkmap); i++) 	\
+		for (i = 0; i < (256 / 32); i++) 	\
 			chunkmap[i] = ~0;			\
 	} while (0)
 
 #define SCTP_CHUNKMAP_COPY(destmap, srcmap) 			\
 	do {							\
 		int i; 						\
-		for (i = 0; i < ELEMCOUNT(chunkmap); i++) 	\
+		for (i = 0; i < (256 / 32); i++) 	\
 			destmap[i] = srcmap[i];			\
 	} while (0)
 
@@ -81,7 +79,7 @@ struct ipt_sctp_info {
 ({							\
 	int i; 						\
 	int flag = 1;					\
-	for (i = 0; i < ELEMCOUNT(chunkmap); i++) {	\
+	for (i = 0; i < (256 / 32); i++) {	\
 		if (chunkmap[i]) {			\
 			flag = 0;			\
 			break;				\
@@ -94,7 +92,7 @@ struct ipt_sctp_info {
 ({							\
 	int i; 						\
 	int flag = 1;					\
-	for (i = 0; i < ELEMCOUNT(chunkmap); i++) {	\
+	for (i = 0; i < (256 / 32); i++) {	\
 		if (chunkmap[i] != ~0) {		\
 			flag = 0;			\
 				break;			\
Index: extensions/libipt_sctp.c
===================================================================
--- extensions/libipt_sctp.c	(revision 6882)
+++ extensions/libipt_sctp.c	(working copy)
@@ -66,7 +66,7 @@ static void help(void)
 " --dport ...\n" 
 " --chunk-types [!] (all|any|none) (chunktype[:flags])+	match if all, any or none of\n"
 "						        chunktypes are present\n"
-"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE ASCONF ASCONF_ACK ALL NONE\n",
+"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE FTSN ASCONF ASCONF_ACK ALL NONE\n",
 	IPTABLES_VERSION);
 }
 
@@ -128,8 +128,9 @@ static struct sctp_chunk_names sctp_chun
     { .name = "ECN_ECNE",	.chunk_type = 12,  .valid_flags = "--------"},
     { .name = "ECN_CWR",	.chunk_type = 13,  .valid_flags = "--------"},
     { .name = "SHUTDOWN_COMPLETE", .chunk_type = 14,  .valid_flags = "-------T"},
-    { .name = "ASCONF",		.chunk_type = 31,  .valid_flags = "--------"},
-    { .name = "ASCONF_ACK",	.chunk_type = 30,  .valid_flags = "--------"},
+    { .name = "FTSN",		.chunk_type = 0xC0,  .valid_flags = "--------"},
+    { .name = "ASCONF",		.chunk_type = 0xC1,  .valid_flags = "--------"},
+    { .name = "ASCONF_ACK",	.chunk_type = 0x80,  .valid_flags = "--------"},
 };
 
 static void
@@ -402,14 +403,14 @@ print_chunk(u_int32_t chunknum, int nume
 
 		for (i = 0; i < ELEMCOUNT(sctp_chunk_names); i++) {
 			if (sctp_chunk_names[i].chunk_type == chunknum)
-				printf("%s", sctp_chunk_names[chunknum].name);
+				printf("%s", sctp_chunk_names[i].name);
 		}
 	}
 }
 
 static void
 print_chunks(u_int32_t chunk_match_type, 
-	     const u_int32_t *chunkmap, 
+	     const u_int32_t *chunkmap,
 	     const struct ipt_sctp_flag_info *flag_info,
 	     int flag_count,
 	     int numeric)
Index: extensions/libip6t_sctp.c
===================================================================
--- extensions/libip6t_sctp.c	(revision 6882)
+++ extensions/libip6t_sctp.c	(working copy)
@@ -66,7 +66,7 @@ static void help(void)
 " --dport ...\n" 
 " --chunk-types [!] (all|any|none) (chunktype[:flags])+	match if all, any or none of\n"
 "						        chunktypes are present\n"
-"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE ASCONF ASCONF_ACK ALL NONE\n",
+"chunktypes - DATA INIT INIT_ACK SACK HEARTBEAT HEARTBEAT_ACK ABORT SHUTDOWN SHUTDOWN_ACK ERROR COOKIE_ECHO COOKIE_ACK ECN_ECNE ECN_CWR SHUTDOWN_COMPLETE FTSN ASCONF ASCONF_ACK ALL NONE\n",
 	IPTABLES_VERSION);
 }
 
@@ -128,8 +128,9 @@ static struct sctp_chunk_names sctp_chun
     { .name = "ECN_ECNE",	.chunk_type = 12,  .valid_flags = "--------"},
     { .name = "ECN_CWR",	.chunk_type = 13,  .valid_flags = "--------"},
     { .name = "SHUTDOWN_COMPLETE", .chunk_type = 14,  .valid_flags = "-------T"},
-    { .name = "ASCONF",		.chunk_type = 31,  .valid_flags = "--------"},
-    { .name = "ASCONF_ACK",	.chunk_type = 30,  .valid_flags = "--------"},
+    { .name = "FTSN",		.chunk_type = 0xC0,  .valid_flags = "--------"},
+    { .name = "ASCONF",		.chunk_type = 0xC1,  .valid_flags = "--------"},
+    { .name = "ASCONF_ACK",	.chunk_type = 0x80,  .valid_flags = "--------"},
 };
 
 static void
@@ -402,7 +403,7 @@ print_chunk(u_int32_t chunknum, int nume
 
 		for (i = 0; i < ELEMCOUNT(sctp_chunk_names); i++) {
 			if (sctp_chunk_names[i].chunk_type == chunknum)
-				printf("%s", sctp_chunk_names[chunknum].name);
+				printf("%s", sctp_chunk_names[i].name);
 		}
 	}
 }

Best regards,
Stephane

Re: [bug+patch] SCTP and chunk types over 0x1f

[Patrick McHardy]

Stephane Chazelas wrote:
> On Mon, Jun 25, 2007 at 03:40:27PM +0200, Patrick McHardy wrote:
> 
>>>- the chunkmap, which is a bitmap of which chunk is selected is
>>>  defined as an array of 256 / 4 u32s, that is 256 * 8 bits,
>>>  256 / 32 is enough (256 bits).
>>
>>
>>We can't change that since it breaks userspace compatibility.
> 
> 
> I see. But that doesn't look very tidy. Maybe some transitional
> code with a warning in both the kernel and iptables?


We could add a new revision, but still need to keep the old
structure for old binaries. Doesn't seem worth it for this
minor waste of space.


>>Could you please resend without the chunkmap changes? Thanks.
> 
> 
> That's just a matter of removing the first change:


Does it stay completely compatible (modulo fixed bugs) then?

Re: [bug+patch] SCTP and chunk types over 0x1f

[Stephane Chazelas]

On Mon, Jun 25, 2007 at 03:58:43PM +0200, Patrick McHardy wrote:
[...]
> > That's just a matter of removing the first change:
> 
> 
> Does it stay completely compatible (modulo fixed bugs) then?

AFAICT, yes (though I've not tested it). I don't know if the
kernel needs to be rebuilt for the bug to be fixed (that is, I
don't know whether in the kernel code there are some calls to
the SCTP_CHUNKMAP_IS_ALL_SET and cie macros with non-array
arguments, I don't think there are).

I've changed:

-#define bytes(type) (sizeof(type) * 8)
-
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\

to:

+		chunkmap[type / 32] |= 	\
+			1 << (type % 32);	\

Because, I think by definition a u_int32_t has 32 bits while
sizeof() is not meant to return the number of 8bit units (though
it probably does on all the architectures supported by Linux).

In practice, it shouldn't make any difference.

Best regards,
Stephane

Re: [bug+patch] SCTP and chunk types over 0x1f

[Stephane Chazelas]

On Mon, Jun 25, 2007 at 03:08:39PM +0100, Stephane Chazelas wrote:
> On Mon, Jun 25, 2007 at 03:58:43PM +0200, Patrick McHardy wrote:
> [...]
> > > That's just a matter of removing the first change:
> > 
> > 
> > Does it stay completely compatible (modulo fixed bugs) then?
> 
> AFAICT, yes (though I've not tested it). I don't know if the
> kernel needs to be rebuilt for the bug to be fixed (that is, I
> don't know whether in the kernel code there are some calls to
> the SCTP_CHUNKMAP_IS_ALL_SET and cie macros with non-array
> arguments, I don't think there are).
[...]

actually yes, the kernel is affected. See "return
SCTP_CHUNKMAP_IS_CLEAR(chunkmap)" for instance in match_packet()
in net/netfilter/xt_sctp.c, chunkmap is u_int32_t*, so
ARRAY_SIZE(chunkmap) will return
sizeof(u_int32_t*)/sizeof(u_int32_t) that is probably 1 or 2
instead of 64 or 8.

The fixed iptable should work with either a fixed or non-fixed
kernel. However, with a non-fixed kernel, it will not fix the
bug completely.

Cheers,
Stephane

Re: [bug+patch] SCTP and chunk types over 0x1f

[Patrick McHardy]

Stephane Chazelas wrote:
> On Mon, Jun 25, 2007 at 03:08:39PM +0100, Stephane Chazelas wrote:
> 
>>>Does it stay completely compatible (modulo fixed bugs) then?
>>
>>AFAICT, yes (though I've not tested it). I don't know if the
>>kernel needs to be rebuilt for the bug to be fixed (that is, I
>>don't know whether in the kernel code there are some calls to
>>the SCTP_CHUNKMAP_IS_ALL_SET and cie macros with non-array
>>arguments, I don't think there are).
> 
> [...]
> 
> actually yes, the kernel is affected. See "return
> SCTP_CHUNKMAP_IS_CLEAR(chunkmap)" for instance in match_packet()
> in net/netfilter/xt_sctp.c, chunkmap is u_int32_t*, so
> ARRAY_SIZE(chunkmap) will return
> sizeof(u_int32_t*)/sizeof(u_int32_t) that is probably 1 or 2
> instead of 64 or 8.
> 
> The fixed iptable should work with either a fixed or non-fixed
> kernel. However, with a non-fixed kernel, it will not fix the
> bug completely.


Thats fine, bugs in the kernel need a new kernel of course.
The important thing is that userspace compatibility of old
binaries is not affected. So the kernel just needs the
new xt_sctp file?

Re: [bug+patch] SCTP and chunk types over 0x1f

[Stephane Chazelas]

On Mon, Jun 25, 2007 at 04:42:49PM +0200, Patrick McHardy wrote:
> Stephane Chazelas wrote:
> > On Mon, Jun 25, 2007 at 03:08:39PM +0100, Stephane Chazelas wrote:
> > 
> >>>Does it stay completely compatible (modulo fixed bugs) then?
> >>
> >>AFAICT, yes (though I've not tested it). I don't know if the
> >>kernel needs to be rebuilt for the bug to be fixed (that is, I
> >>don't know whether in the kernel code there are some calls to
> >>the SCTP_CHUNKMAP_IS_ALL_SET and cie macros with non-array
> >>arguments, I don't think there are).
> > 
> > [...]
> > 
> > actually yes, the kernel is affected. See "return
> > SCTP_CHUNKMAP_IS_CLEAR(chunkmap)" for instance in match_packet()
> > in net/netfilter/xt_sctp.c, chunkmap is u_int32_t*, so
> > ARRAY_SIZE(chunkmap) will return
> > sizeof(u_int32_t*)/sizeof(u_int32_t) that is probably 1 or 2
> > instead of 64 or 8.
> > 
> > The fixed iptable should work with either a fixed or non-fixed
> > kernel. However, with a non-fixed kernel, it will not fix the
> > bug completely.
> 
> 
> Thats fine, bugs in the kernel need a new kernel of course.
> The important thing is that userspace compatibility of old
> binaries is not affected. So the kernel just needs the
> new xt_sctp file?

Well, no, the new header files to match the ones in iptable, so
include/linux/netfilter/xt_sctp.h and
include/linux/netfilter_ipv4/ipt_sctp.h

(replace the ELEMCOUNT(...) with (256/32))

Or you could fix xt_sctp.c, but then, you'd have a discrepancy
between the header file in the kernel and the header file in
iptables.

Or else, you could fix iptables without changing the header
files maybe by doing some sort of dodgy array type casting (I'm
not sure how you'd do that though).

Cheers,
Stephane