diff mbox series

[1/2] Convert CHECK_BOUND macro to function

Message ID 20171113080941.616-1-eric.sesterhenn@x41-dsec.de
State Accepted
Delegated to: Pablo Neira
Headers show
Series [1/2] Convert CHECK_BOUND macro to function | expand

Commit Message

Eric Sesterhenn Nov. 13, 2017, 8:09 a.m. UTC 
From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>

It is bad practive to return in a macro, this patch
moves the check into a function.

Signed-off-by: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
---
 net/netfilter/nf_conntrack_h323_asn1.c | 94 +++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 29 deletions(-)

Comments

Pablo Neira Ayuso Nov. 13, 2017, 1:13 p.m. UTC | #1 
On Mon, Nov 13, 2017 at 09:09:40AM +0100, eric.sesterhenn@x41-dsec.de wrote:
> From: Eric Sesterhenn <eric.sesterhenn@x41-dsec.de>
> 
> It is bad practive to return in a macro, this patch
> moves the check into a function.

Applied with minor changes, see below.

[...]
> diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
> index 89b2e46925c4..f358222b1e5e 100644
> --- a/net/netfilter/nf_conntrack_h323_asn1.c
> +++ b/net/netfilter/nf_conntrack_h323_asn1.c
> @@ -103,7 +103,6 @@ typedef struct {
>  #define INC_BIT(bs) if((++(bs)->bit)>7){(bs)->cur++;(bs)->bit=0;}
>  #define INC_BITS(bs,b) if(((bs)->bit+=(b))>7){(bs)->cur+=(bs)->bit>>3;(bs)->bit&=7;}
>  #define BYTE_ALIGN(bs) if((bs)->bit){(bs)->cur++;(bs)->bit=0;}
> -#define CHECK_BOUND(bs,n) if((bs)->cur+(n)>(bs)->end)return(H323_ERROR_BOUND)
>  static unsigned int get_len(bitstr_t *bs);
>  static unsigned int get_bit(bitstr_t *bs);
>  static unsigned int get_bits(bitstr_t *bs, unsigned int b);
> @@ -166,6 +165,14 @@ static unsigned int get_len(bitstr_t *bs)
>  }
>  
>  /****************************************************************************/
> +static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes)

Make sure you make you patches on top of nf-next.git:

https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next.git/

Look, bitstr_t is gone there already, we got a patch to remove
typedefs.

Anyway, I have mangled this here and it's now applied.

It would be great if your follow up patch subject is prefixes like
this:

        netfilter: nf_ct_h323: blah

So we know what subsystem this is targeting to, just for the next
time.

Thanks for following up on this!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/net/netfilter/nf_conntrack_h323_asn1.c b/net/netfilter/nf_conntrack_h323_asn1.c
index 89b2e46925c4..f358222b1e5e 100644
--- a/net/netfilter/nf_conntrack_h323_asn1.c
+++ b/net/netfilter/nf_conntrack_h323_asn1.c
@@ -103,7 +103,6 @@  typedef struct {
 #define INC_BIT(bs) if((++(bs)->bit)>7){(bs)->cur++;(bs)->bit=0;}
 #define INC_BITS(bs,b) if(((bs)->bit+=(b))>7){(bs)->cur+=(bs)->bit>>3;(bs)->bit&=7;}
 #define BYTE_ALIGN(bs) if((bs)->bit){(bs)->cur++;(bs)->bit=0;}
-#define CHECK_BOUND(bs,n) if((bs)->cur+(n)>(bs)->end)return(H323_ERROR_BOUND)
 static unsigned int get_len(bitstr_t *bs);
 static unsigned int get_bit(bitstr_t *bs);
 static unsigned int get_bits(bitstr_t *bs, unsigned int b);
@@ -166,6 +165,14 @@  static unsigned int get_len(bitstr_t *bs)
 }
 
 /****************************************************************************/
+static int nf_h323_error_boundary(bitstr_t *bs, size_t bytes)
+{
+	if(*bs->cur + bytes > *bs->end)
+		return 1;
+	return 0;
+}
+
+/****************************************************************************/
 static unsigned int get_bit(bitstr_t *bs)
 {
 	unsigned int b = (*bs->cur) & (0x80 >> bs->bit);
@@ -280,7 +287,8 @@  static int decode_bool(bitstr_t *bs, const struct field_t *f,
 
 	INC_BIT(bs);
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -293,11 +301,14 @@  static int decode_oid(bitstr_t *bs, const struct field_t *f,
 	PRINT("%*.s%s\n", level * TAB_SIZE, " ", f->name);
 
 	BYTE_ALIGN(bs);
-	CHECK_BOUND(bs, 1);
+	if (nf_h323_error_boundary(bs, 1))
+		return H323_ERROR_BOUND;
+
 	len = *bs->cur++;
 	bs->cur += len;
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 
-	CHECK_BOUND(bs, 0);
 	return H323_ERROR_NONE;
 }
 
@@ -330,7 +341,8 @@  static int decode_int(bitstr_t *bs, const struct field_t *f,
 		break;
 	case UNCO:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
 		bs->cur += len;
 		break;
@@ -341,7 +353,8 @@  static int decode_int(bitstr_t *bs, const struct field_t *f,
 
 	PRINT("\n");
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -357,7 +370,8 @@  static int decode_enum(bitstr_t *bs, const struct field_t *f,
 		INC_BITS(bs, f->sz);
 	}
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -375,12 +389,14 @@  static int decode_bitstr(bitstr_t *bs, const struct field_t *f,
 		len = f->lb;
 		break;
 	case WORD:		/* 2-byte length */
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = (*bs->cur++) << 8;
 		len += (*bs->cur++) + f->lb;
 		break;
 	case SEMI:
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
 		break;
 	default:
@@ -391,7 +407,8 @@  static int decode_bitstr(bitstr_t *bs, const struct field_t *f,
 	bs->cur += len >> 3;
 	bs->bit = len & 7;
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -409,7 +426,8 @@  static int decode_numstr(bitstr_t *bs, const struct field_t *f,
 	BYTE_ALIGN(bs);
 	INC_BITS(bs, (len << 2));
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -440,12 +458,14 @@  static int decode_octstr(bitstr_t *bs, const struct field_t *f,
 		break;
 	case BYTE:		/* Range == 256 */
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 1);
+		if (nf_h323_error_boundary(bs, 1))
+			return H323_ERROR_BOUND;
 		len = (*bs->cur++) + f->lb;
 		break;
 	case SEMI:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs) + f->lb;
 		break;
 	default:		/* 2 <= Range <= 255 */
@@ -458,7 +478,8 @@  static int decode_octstr(bitstr_t *bs, const struct field_t *f,
 
 	PRINT("\n");
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -473,7 +494,8 @@  static int decode_bmpstr(bitstr_t *bs, const struct field_t *f,
 	switch (f->sz) {
 	case BYTE:		/* Range == 256 */
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 1);
+		if (nf_h323_error_boundary(bs, 1))
+			return H323_ERROR_BOUND;
 		len = (*bs->cur++) + f->lb;
 		break;
 	default:		/* 2 <= Range <= 255 */
@@ -484,7 +506,8 @@  static int decode_bmpstr(bitstr_t *bs, const struct field_t *f,
 
 	bs->cur += len << 1;
 
-	CHECK_BOUND(bs, 0);
+	if (nf_h323_error_boundary(bs, 0))
+		return H323_ERROR_BOUND;
 	return H323_ERROR_NONE;
 }
 
@@ -525,9 +548,11 @@  static int decode_seq(bitstr_t *bs, const struct field_t *f,
 
 		/* Decode */
 		if (son->attr & OPEN) {	/* Open field */
-			CHECK_BOUND(bs, 2);
+			if (nf_h323_error_boundary(bs, 2))
+				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			CHECK_BOUND(bs, len);
+			if (nf_h323_error_boundary(bs, len))
+				return H323_ERROR_BOUND;
 			if (!base || !(son->attr & DECODE)) {
 				PRINT("%*.s%s\n", (level + 1) * TAB_SIZE,
 				      " ", son->name);
@@ -556,7 +581,8 @@  static int decode_seq(bitstr_t *bs, const struct field_t *f,
 
 	/* Get the extension bitmap */
 	bmp2_len = get_bits(bs, 7) + 1;
-	CHECK_BOUND(bs, (bmp2_len + 7) >> 3);
+	if (nf_h323_error_boundary(bs, (bmp2_len + 7) >> 3))
+		return H323_ERROR_BOUND;
 	bmp2 = get_bitmap(bs, bmp2_len);
 	bmp |= bmp2 >> f->sz;
 	if (base)
@@ -567,9 +593,11 @@  static int decode_seq(bitstr_t *bs, const struct field_t *f,
 	for (opt = 0; opt < bmp2_len; opt++, i++, son++) {
 		/* Check Range */
 		if (i >= f->ub) {	/* Newer Version? */
-			CHECK_BOUND(bs, 2);
+			if (nf_h323_error_boundary(bs, 2))
+				return H323_ERROR_BOUND;
 			len = get_len(bs);
-			CHECK_BOUND(bs, len);
+			if (nf_h323_error_boundary(bs, len))
+				return H323_ERROR_BOUND;
 			bs->cur += len;
 			continue;
 		}
@@ -583,9 +611,11 @@  static int decode_seq(bitstr_t *bs, const struct field_t *f,
 		if (!((0x80000000 >> opt) & bmp2))	/* Not present */
 			continue;
 
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		len = get_len(bs);
-		CHECK_BOUND(bs, len);
+		if (nf_h323_error_boundary(bs, len))
+			return H323_ERROR_BOUND;
 		if (!base || !(son->attr & DECODE)) {
 			PRINT("%*.s%s\n", (level + 1) * TAB_SIZE, " ",
 			      son->name);
@@ -623,19 +653,22 @@  static int decode_seqof(bitstr_t *bs, const struct field_t *f,
 	switch (f->sz) {
 	case BYTE:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 1);
+		if (nf_h323_error_boundary(bs, 1))
+			return H323_ERROR_BOUND;
 		count = *bs->cur++;
 		break;
 	case WORD:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		count = *bs->cur++;
 		count <<= 8;
 		count += *bs->cur++;
 		break;
 	case SEMI:
 		BYTE_ALIGN(bs);
-		CHECK_BOUND(bs, 2);
+		if (nf_h323_error_boundary(bs, 2))
+			return H323_ERROR_BOUND;
 		count = get_len(bs);
 		break;
 	default:
@@ -659,7 +692,8 @@  static int decode_seqof(bitstr_t *bs, const struct field_t *f,
 		if (son->attr & OPEN) {
 			BYTE_ALIGN(bs);
 			len = get_len(bs);
-			CHECK_BOUND(bs, len);
+			if (nf_h323_error_boundary(bs, len))
+				return H323_ERROR_BOUND;
 			if (!base || !(son->attr & DECODE)) {
 				PRINT("%*.s%s\n", (level + 1) * TAB_SIZE,
 				      " ", son->name);
@@ -728,7 +762,8 @@  static int decode_choice(bitstr_t *bs, const struct field_t *f,
 	if (type >= f->ub) {	/* Newer version? */
 		BYTE_ALIGN(bs);
 		len = get_len(bs);
-		CHECK_BOUND(bs, len);
+		if (nf_h323_error_boundary(bs, len))
+			return H323_ERROR_BOUND;
 		bs->cur += len;
 		return H323_ERROR_NONE;
 	}
@@ -743,7 +778,8 @@  static int decode_choice(bitstr_t *bs, const struct field_t *f,
 	if (ext || (son->attr & OPEN)) {
 		BYTE_ALIGN(bs);
 		len = get_len(bs);
-		CHECK_BOUND(bs, len);
+		if (nf_h323_error_boundary(bs, len))
+			return H323_ERROR_BOUND;
 		if (!base || !(son->attr & DECODE)) {
 			PRINT("%*.s%s\n", (level + 1) * TAB_SIZE, " ",
 			      son->name);