sctp: chunkmap size is too large

PATCH In review Olivier MATZ 4 participants 5 messages Jan 30, 2009 – Feb 9, 2009

Subsystems (3)

Networking Network drivers netfilter

lore

Share & tools

lore

Olivier MATZ

author

2009-01-30 15:43 UTC #0 lore raw

PATCH 1/1 sctp: chunkmap size is too large

Hi all,

I think that the chunkmap field in struct xt_sctp_info is too large: we have 256 chunk types and it's a bitfield, so the table should be 256 bits (8 uint32_t) instead of 64 uint32_t.

I updated the size of the table, and I did some cosmetic changes in SCTP_CHUNKMAP_* macros (use a mask instead of a modulo).

 netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
 netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
 2 files changed, 32 insertions(+), 28 deletions(-)

Can someone have a look at it ? Please CC me for any replies as I'm not on the list.

Thanks,
Olivier

include/linux/netfilter/xt_sctp.h +16 ~~-14~~

diff -r ce4087e26684 include/linux/netfilter/xt_sctp.h
@@ -15,11 +15,13 @@
 
 #define XT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 struct xt_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 / sizeof_bits(u_int32_t)];  /* 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 */

@@ -33,24 +35,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		(chunkmap)[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		(chunkmap)[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	((chunkmap)[type / bytes (u_int32_t)] & 		\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) \

include/linux/netfilter_ipv4/ipt_sctp.h +16 ~~-14~~

diff -r ce4087e26684 include/linux/netfilter_ipv4/ipt_sctp.h
@@ -16,11 +16,13 @@
 
 #define IPT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 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 / sizeof_bits(u_int32_t)];  /* 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 */

@@ -34,24 +36,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		chunkmap[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	(chunkmap[type / bytes (u_int32_t)] & 			\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) 				\

David Miller

maintainer

2009-02-01 09:14 UTC #1 lore raw

From: Olivier MATZ <[email protected]>
Date: Fri, 30 Jan 2009 16:43:07 +0100

Quoted 9 lines

I think that the chunkmap field in struct xt_sctp_info is too large: we have 256 chunk types and it's a bitfield, so the table should be 256 bits (8 uint32_t) instead of 64 uint32_t.
I updated the size of the table, and I did some cosmetic changes in SCTP_CHUNKMAP_* macros (use a mask instead of a modulo).

Quoted 6 lines

  netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
  netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
  2 files changed, 32 insertions(+), 28 deletions(-)
Can someone have a look at it ? Please CC me for any replies as I'm not on the list.

Patrick, I assume you got this?

Jan Engelhardt

2009-02-04 10:12 UTC #2 lore raw

Quoted 9 lines · David Miller

Quoted 9 lines
I think that the chunkmap field in struct xt_sctp_info is too large: we have 256 chunk types and it's a bitfield, so the table should be 256 bits (8 uint32_t) instead of 64 uint32_t.
I updated the size of the table, and I did some cosmetic changes in SCTP_CHUNKMAP_* macros (use a mask instead of a modulo).

Quoted 8 lines

Quoted 6 lines
  netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
  netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
  2 files changed, 32 insertions(+), 28 deletions(-)
Can someone have a look at it ? Please CC me for any replies as I'm not on the list.
Patrick, I assume you got this?

At least I got this - and the change would have to be nak'ed because it changes the userspace<->kernel binary stream. If you do such a change, a new match revision is required.

Patrick McHardy

2009-02-09 13:50 UTC #3 lore raw

In reply to David Miller

Quoted 12 lines · David Miller

From: Olivier MATZ <[email protected]>
Date: Fri, 30 Jan 2009 16:43:07 +0100
Quoted 9 lines
I think that the chunkmap field in struct xt_sctp_info is too large: we have 256 chunk types and it's a bitfield, so the table should be 256 bits (8 uint32_t) instead of 64 uint32_t.
I updated the size of the table, and I did some cosmetic changes in SCTP_CHUNKMAP_* macros (use a mask instead of a modulo).

Quoted 8 lines

Quoted 6 lines
  netfilter/xt_sctp.h       |   30 ++++++++++++++++--------------
  netfilter_ipv4/ipt_sctp.h |   30 ++++++++++++++++--------------
  2 files changed, 32 insertions(+), 28 deletions(-)
Can someone have a look at it ? Please CC me for any replies as I'm not on the list.
Patrick, I assume you got this?

Sorry for the delay. As Jan already noticed, it changes the ABI, so we change it without adding a new revision. And I don't think a few bytes of memory savings justify that.

Olivier MATZ

author

2009-02-02 09:23 UTC #4 lore raw

In reply to Olivier MATZ

Hi,

A file was missing in the previous patch. I think that the size of chunkmapcopy in xt_sctp.c should also be fixed.

I attached the new patch.

 include/linux/netfilter/xt_sctp.h       |   30 
++++++++++++++++--------------
 include/linux/netfilter_ipv4/ipt_sctp.h |   30 
++++++++++++++++--------------
 net/netfilter/xt_sctp.c                 |    2 +-
 3 files changed, 33 insertions(+), 29 deletions(-)

Signed-off-by Olivier Matz <[email protected]>

Thanks,
Olivier

include/linux/netfilter/xt_sctp.h +16 ~~-14~~

diff -r 94166a3a38bd include/linux/netfilter/xt_sctp.h
@@ -15,11 +15,13 @@
 
 #define XT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 struct xt_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 / sizeof_bits(u_int32_t)];  /* 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 */

@@ -33,24 +35,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		(chunkmap)[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		(chunkmap)[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	((chunkmap)[type / bytes (u_int32_t)] & 		\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) \

include/linux/netfilter_ipv4/ipt_sctp.h +16 ~~-14~~

diff -r 94166a3a38bd include/linux/netfilter_ipv4/ipt_sctp.h
@@ -16,11 +16,13 @@
 
 #define IPT_NUM_SCTP_FLAGS	4
 
+#define sizeof_bits(type) (sizeof(type) * 8)
+
 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 / sizeof_bits(u_int32_t)];  /* 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 */

@@ -34,24 +36,24 @@
 	u_int32_t invflags;
 };
 
-#define bytes(type) (sizeof(type) * 8)
+#define SCTP_MODULO(chunktype, type) (chunktype & (sizeof_bits(type)-1))
 
-#define SCTP_CHUNKMAP_SET(chunkmap, type) 		\
-	do { 						\
-		chunkmap[type / bytes(u_int32_t)] |= 	\
-			1 << (type % bytes(u_int32_t));	\
+#define SCTP_CHUNKMAP_SET(chunkmap, chunktype)				\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] |=		\
+			1 << SCTP_MODULO(chunktype, u_int32_t);		\
 	} while (0)
 
-#define SCTP_CHUNKMAP_CLEAR(chunkmap, type)		 	\
-	do {							\
-		chunkmap[type / bytes(u_int32_t)] &= 		\
-			~(1 << (type % bytes(u_int32_t)));	\
+#define SCTP_CHUNKMAP_CLEAR(chunkmap, chunktype)		 	\
+	do {								\
+		chunkmap[chunktype / sizeof_bits(u_int32_t)] &=		\
+			~(1 << SCTP_MODULO(chunktype, u_int32_t));	\
 	} while (0)
 
-#define SCTP_CHUNKMAP_IS_SET(chunkmap, type) 			\
-({								\
-	(chunkmap[type / bytes (u_int32_t)] & 			\
-		(1 << (type % bytes (u_int32_t)))) ? 1: 0;	\
+#define SCTP_CHUNKMAP_IS_SET(chunkmap, chunktype) 			\
+({									\
+	(chunkmap[chunktype / sizeof_bits(u_int32_t)] &			\
+	 (1 << SCTP_MODULO(chunktype, u_int32_t))) ? 1 : 0;		\
 })
 
 #define SCTP_CHUNKMAP_RESET(chunkmap) 				\

net/netfilter/xt_sctp.c +1 -1

diff -r 94166a3a38bd net/netfilter/xt_sctp.c
@@ -45,7 +45,7 @@
 	     const struct xt_sctp_info *info,
 	     bool *hotdrop)
 {
-	u_int32_t chunkmapcopy[256 / sizeof (u_int32_t)];
+	u_int32_t chunkmapcopy[256 / sizeof_bits(u_int32_t)];
 	const sctp_chunkhdr_t *sch;
 	sctp_chunkhdr_t _sch;
 	int chunk_match_type = info->chunk_match_type;