[RFC PATCH v2 1/4] ppp: adds new error for decompressors to signal that packet must be dropped

[RFC PATCH v2 1/4] ppp: adds new error for decompressors to signal that packet must be dropped

[Jorge Boncompte [DTI2]]

From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>

Currently decompressors can't signal the generic PPP layer to silently
drop a packet without notifying the PPP daemon or the other party.

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
 drivers/net/ppp/ppp_generic.c |   12 ++++++++++++
 include/linux/ppp-comp.h      |    6 ++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 72ff14b..7d26825 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -1729,6 +1729,10 @@ ppp_receive_nonmp_frame(struct ppp *ppp, struct sk_buff *skb)
 	    (ppp->rstate & (SC_DC_FERROR | SC_DC_ERROR)) = 0)
 		skb = ppp_decompress_frame(ppp, skb);
 
+	/* Packet dropped */
+	if (skb = NULL)
+		goto err;
+
 	if (ppp->flags & SC_MUST_COMP && ppp->rstate & SC_DC_FERROR)
 		goto err;
 
@@ -1888,6 +1892,13 @@ ppp_decompress_frame(struct ppp *ppp, struct sk_buff *skb)
 		len = ppp->rcomp->decompress(ppp->rc_state, skb->data - 2,
 				skb->len + 2, ns->data, obuff_size);
 		if (len < 0) {
+			/* Drop the packet and continue */
+			if (len = DECOMP_DROPERROR) {
+				kfree_skb(ns);
+				kfree_skb(skb);
+				skb = NULL;
+				goto out;
+			}
 			/* Pass the compressed frame to pppd as an
 			   error indication. */
 			if (len = DECOMP_FATALERROR)
@@ -1909,6 +1920,7 @@ ppp_decompress_frame(struct ppp *ppp, struct sk_buff *skb)
 					   skb->len + 2);
 	}
 
+out:
 	return skb;
 
  err:
diff --git a/include/linux/ppp-comp.h b/include/linux/ppp-comp.h
index 4ea1d37..12a8ce8 100644
--- a/include/linux/ppp-comp.h
+++ b/include/linux/ppp-comp.h
@@ -89,8 +89,9 @@ struct compressor {
 /*
  * The return value from decompress routine is the length of the
  * decompressed packet if successful, otherwise DECOMP_ERROR
- * or DECOMP_FATALERROR if an error occurred.
- * 
+ * or DECOMP_FATALERROR if an error occurred but don't want the
+ * PPP generic layer to drop the packet.
+ *
  * We need to make this distinction so that we can disable certain
  * useful functionality, namely sending a CCP reset-request as a result
  * of an error detected after decompression.  This is to avoid infringing
@@ -100,6 +101,7 @@ struct compressor {
 
 #define DECOMP_ERROR		-1	/* error detected before decomp. */
 #define DECOMP_FATALERROR	-2	/* error detected after decomp. */
+#define DECOMP_DROPERROR	-3	/* error detected, drop packet. */
 
 extern int ppp_register_compressor(struct compressor *);
 extern void ppp_unregister_compressor(struct compressor *);
-- 
1.7.10.4

[RFC PATCH v2 2/4] ppp_mppe: check coherency counter for out-of-order sequencing

[Jorge Boncompte [DTI2]]

From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>

While testing a L2TP tunnel without sequencing with MPPE encryption in
stateless mode noticed that after a packet was reordered the encapsulated
traffic session was stuck but testing against a Cisco gear did work.

From RFC3078 "MPPE expects packets to be delivered in sequence".

The thing it's that the ppp_mppe module treats the reorder as if the
coherency counter did wrap and rekeys all the "missing" packets.

The link layer protocol should deliver the packets in order but at least
with this patch in place the decryption process survives some packet reorder.

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
 drivers/net/ppp/ppp_mppe.c |   23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 9a1849a..97f4804 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -55,6 +55,7 @@
 #include <linux/ppp_defs.h>
 #include <linux/ppp-comp.h>
 #include <linux/scatterlist.h>
+#include <linux/net.h>
 #include <asm/unaligned.h>
 
 #include "ppp_mppe.h"
@@ -125,7 +126,10 @@ struct ppp_mppe_state {
 
 #define MPPE_BITS(p) ((p)[4] & 0xf0)
 #define MPPE_CCOUNT(p) ((((p)[4] & 0x0f) << 8) + (p)[5])
-#define MPPE_CCOUNT_SPACE 0x1000	/* The size of the ccount space */
+#define MPPE_CCOUNT_BITS 12	/* MPPE coherency count bits */
+/* The size of the ccount space */
+#define MPPE_CCOUNT_SPACE (1 << MPPE_CCOUNT_BITS)
+#define MPPE_CCOUNT_SHIFT ((sizeof(int) * 8) - MPPE_CCOUNT_BITS)
 
 #define MPPE_OVHD	2	/* MPPE overhead/packet */
 #define SANITY_MAX	1600	/* Max bogon factor we will tolerate */
@@ -469,6 +473,14 @@ static void mppe_decomp_reset(void *arg)
 }
 
 /*
+ * Compares two coherency counter values.
+ */
+static int mppe_cmp_ccount(unsigned int a, unsigned int b)
+{
+	return (int)((a << MPPE_CCOUNT_SHIFT) - (b << MPPE_CCOUNT_SHIFT));
+}
+
+/*
  * Decompress (decrypt) an MPPE packet.
  */
 static int
@@ -547,6 +559,15 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	 */
 
 	if (!state->stateful) {
+		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
+			if (net_ratelimit())
+				printk(KERN_WARNING "%s[%d]: Dropping out-of-order "
+				       "packet with ccount %u, "
+				       "expecting %u!\n", __func__,
+				       state->unit, ccount, state->ccount);
+			return DECOMP_DROPERROR;
+		}
+
 		/* RFC 3078, sec 8.1.  Rekey for every packet. */
 		while (state->ccount != ccount) {
 			mppe_rekey(state, 0);
-- 
1.7.10.4

[RFC PATCH v2 3/4] ppp_mppe: cleanup kernel log messages

[Jorge Boncompte [DTI2]]

From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>

- Consolidate log messages, print PPP unit number where available.
- Changes error or warning messages to correct log level.
- Use _ratelimited() functions for messages triggered by network packets.

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
 drivers/net/ppp/ppp_mppe.c |   78 +++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 97f4804..54c64cf 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -178,7 +178,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
 		setup_sg(sg_out, state->session_key, state->keylen);
 		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
 					     state->keylen) != 0) {
-    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
+			net_warn_ratelimited("%s[%d]: crypto_blkcipher_encrypt "
+					     "failed\n", __func__, state->unit);
 		}
 	} else {
 		memcpy(state->session_key, state->sha1_digest, state->keylen);
@@ -291,8 +292,7 @@ mppe_init(void *arg, unsigned char *options, int optlen, int unit, int debug,
 	else if (mppe_opts & MPPE_OPT_40)
 		state->keylen = 8;
 	else {
-		printk(KERN_WARNING "%s[%d]: unknown key length\n", debugstr,
-		       unit);
+		pr_warn("%s[%d]: unknown key length\n", debugstr, unit);
 		return 0;
 	}
 	if (mppe_opts & MPPE_OPT_STATEFUL)
@@ -314,8 +314,7 @@ mppe_init(void *arg, unsigned char *options, int optlen, int unit, int debug,
 			sprintf(mkey + i * 2, "%02x", state->master_key[i]);
 		for (i = 0; i < sizeof(state->session_key); i++)
 			sprintf(skey + i * 2, "%02x", state->session_key[i]);
-		printk(KERN_DEBUG
-		       "%s[%d]: keys: master: %s initial session: %s\n",
+		printk(KERN_DEBUG "%s[%d]: keys: master: %s initial session: %s\n",
 		       debugstr, unit, mkey, skey);
 	}
 
@@ -387,9 +386,9 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
 	/* Make sure we have enough room to generate an encrypted packet. */
 	if (osize < isize + MPPE_OVHD + 2) {
 		/* Drop the packet if we should encrypt it, but can't. */
-		printk(KERN_DEBUG "mppe_compress[%d]: osize too small! "
-		       "(have: %d need: %d)\n", state->unit,
-		       osize, osize + MPPE_OVHD + 2);
+		net_err_ratelimited("%s[%d]: osize too small! "
+				    "(have: %d need: %d)\n", __func__,
+				    state->unit, osize, isize + MPPE_OVHD + 2);
 		return -1;
 	}
 
@@ -405,7 +404,7 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
 
 	state->ccount = (state->ccount + 1) % MPPE_CCOUNT_SPACE;
 	if (state->debug >= 7)
-		printk(KERN_DEBUG "mppe_compress[%d]: ccount %d\n", state->unit,
+		printk(KERN_DEBUG "%s[%d]: ccount %d\n", __func__, state->unit,
 		       state->ccount);
 	put_unaligned_be16(state->ccount, obuf);
 
@@ -414,7 +413,7 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
 	    (state->bits & MPPE_BIT_FLUSHED)) {	/* CCP Reset-Request  */
 		/* We must rekey */
 		if (state->debug && state->stateful)
-			printk(KERN_DEBUG "mppe_compress[%d]: rekeying\n",
+			printk(KERN_DEBUG "%s[%d]: rekeying\n", __func__,
 			       state->unit);
 		mppe_rekey(state, 0);
 		state->bits |= MPPE_BIT_FLUSHED;
@@ -432,7 +431,8 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
 	setup_sg(sg_in, ibuf, isize);
 	setup_sg(sg_out, obuf, osize);
 	if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in, isize) != 0) {
-		printk(KERN_DEBUG "crypto_cypher_encrypt failed\n");
+		net_err_ratelimited("%s[%d]: crypto_blkcipher_encrypt failed\n",
+				    __func__, state->unit);
 		return -1;
 	}
 
@@ -496,9 +496,8 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 
 	if (isize <= PPP_HDRLEN + MPPE_OVHD) {
 		if (state->debug)
-			printk(KERN_DEBUG
-			       "mppe_decompress[%d]: short pkt (%d)\n",
-			       state->unit, isize);
+			printk(KERN_DEBUG "%s[%d]: short pkt (%d)\n",
+			       __func__, state->unit, isize);
 		return DECOMP_ERROR;
 	}
 
@@ -509,35 +508,36 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	 * this is to account for possible PFC.
 	 */
 	if (osize < isize - MPPE_OVHD - 1) {
-		printk(KERN_DEBUG "mppe_decompress[%d]: osize too small! "
-		       "(have: %d need: %d)\n", state->unit,
-		       osize, isize - MPPE_OVHD - 1);
+		net_warn_ratelimited("%s[%d]: osize too small! "
+				     "(have: %d need: %d)\n", __func__,
+				     state->unit, osize, isize - MPPE_OVHD - 1);
 		return DECOMP_ERROR;
 	}
 	osize = isize - MPPE_OVHD - 2;	/* assume no PFC */
 
 	ccount = MPPE_CCOUNT(ibuf);
 	if (state->debug >= 7)
-		printk(KERN_DEBUG "mppe_decompress[%d]: ccount %d\n",
-		       state->unit, ccount);
+		printk(KERN_DEBUG "%s[%d]: ccount %d\n", __func__, state->unit,
+		       ccount);
 
 	/* sanity checks -- terminate with extreme prejudice */
 	if (!(MPPE_BITS(ibuf) & MPPE_BIT_ENCRYPTED)) {
-		printk(KERN_DEBUG
-		       "mppe_decompress[%d]: ENCRYPTED bit not set!\n",
-		       state->unit);
+		net_warn_ratelimited("%s[%d]: ENCRYPTED bit not set!\n",
+				     __func__, state->unit);
 		state->sanity_errors += 100;
 		sanity = 1;
 	}
 	if (!state->stateful && !flushed) {
-		printk(KERN_DEBUG "mppe_decompress[%d]: FLUSHED bit not set in "
-		       "stateless mode!\n", state->unit);
+		net_warn_ratelimited("%s[%d]: FLUSHED bit not set "
+				     "in stateless mode!\n", __func__,
+				     state->unit);
 		state->sanity_errors += 100;
 		sanity = 1;
 	}
 	if (state->stateful && ((ccount & 0xff) = 0xff) && !flushed) {
-		printk(KERN_DEBUG "mppe_decompress[%d]: FLUSHED bit not set on "
-		       "flag packet!\n", state->unit);
+		net_warn_ratelimited("%s[%d]: FLUSHED bit not set "
+				     "on flag packet!\n", __func__,
+				     state->unit);
 		state->sanity_errors += 100;
 		sanity = 1;
 	}
@@ -560,11 +560,11 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 
 	if (!state->stateful) {
 		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
-			if (net_ratelimit())
-				printk(KERN_WARNING "%s[%d]: Dropping out-of-order "
-				       "packet with ccount %u, "
-				       "expecting %u!\n", __func__,
-				       state->unit, ccount, state->ccount);
+			net_warn_ratelimited("%s[%d]: Dropping out-of-order "
+					     "packet with ccount %u, "
+					     "expecting %u!\n", __func__,
+					     state->unit, ccount,
+					     state->ccount);
 			return DECOMP_DROPERROR;
 		}
 
@@ -638,7 +638,9 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	setup_sg(sg_in, ibuf, 1);
 	setup_sg(sg_out, obuf, 1);
 	if (crypto_blkcipher_decrypt(&desc, sg_out, sg_in, 1) != 0) {
-		printk(KERN_DEBUG "crypto_cypher_decrypt failed\n");
+		net_warn_ratelimited
+		    ("%s[%d]: crypto_blkcipher_decrypt failed\n", __func__,
+		     state->unit);
 		return DECOMP_ERROR;
 	}
 
@@ -658,7 +660,9 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	setup_sg(sg_in, ibuf + 1, isize - 1);
 	setup_sg(sg_out, obuf + 1, osize - 1);
 	if (crypto_blkcipher_decrypt(&desc, sg_out, sg_in, isize - 1)) {
-		printk(KERN_DEBUG "crypto_cypher_decrypt failed\n");
+		net_warn_ratelimited
+		    ("%s[%d]: crypto_blkcipher_decrypt failed\n", __func__,
+		     state->unit);
 		return DECOMP_ERROR;
 	}
 
@@ -685,9 +689,9 @@ static void mppe_incomp(void *arg, unsigned char *ibuf, int icnt)
 
 	if (state->debug &&
 	    (PPP_PROTOCOL(ibuf) >= 0x0021 && PPP_PROTOCOL(ibuf) <= 0x00fa))
-		printk(KERN_DEBUG
-		       "mppe_incomp[%d]: incompressible (unencrypted) data! "
-		       "(proto %04x)\n", state->unit, PPP_PROTOCOL(ibuf));
+		printk(KERN_DEBUG "%s[%d]: incompressible (unencrypted) data! "
+		       "(proto %04x)\n", __func__, state->unit,
+		       PPP_PROTOCOL(ibuf));
 
 	state->stats.inc_bytes += icnt;
 	state->stats.inc_packets++;
@@ -744,7 +748,7 @@ static int __init ppp_mppe_init(void)
 	answer = ppp_register_compressor(&ppp_mppe);
 
 	if (answer = 0)
-		printk(KERN_INFO "PPP MPPE Compression module registered\n");
+		pr_info("PPP MPPE Compression module registered\n");
 	else
 		kfree(sha_pad);
 
-- 
1.7.10.4

[RFC PATCH v2 4/4] ppp_mppe: style cleanup

[Jorge Boncompte [DTI2]]

From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>

Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
---
 drivers/net/ppp/ppp_mppe.c |  104 +++++++++++++++++++++-----------------------
 1 file changed, 49 insertions(+), 55 deletions(-)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 54c64cf..4d27245 100644
--- a/drivers/net/ppp/ppp_mppe.c
+++ b/drivers/net/ppp/ppp_mppe.c
@@ -66,8 +66,8 @@ MODULE_LICENSE("Dual BSD/GPL");
 MODULE_ALIAS("ppp-compress-" __stringify(CI_MPPE));
 MODULE_VERSION("1.0.2");
 
-static unsigned int
-setup_sg(struct scatterlist *sg, const void *address, unsigned int length)
+static unsigned int setup_sg(struct scatterlist *sg, const void *address,
+			     unsigned int length)
 {
 	sg_set_buf(sg, address, length);
 	return length;
@@ -138,7 +138,7 @@ struct ppp_mppe_state {
  * Key Derivation, from RFC 3078, RFC 3079.
  * Equivalent to Get_Key() for MS-CHAP as described in RFC 3079.
  */
-static void get_new_key_from_sha(struct ppp_mppe_state * state)
+static void get_new_key_from_sha(struct ppp_mppe_state *state)
 {
 	struct hash_desc desc;
 	struct scatterlist sg[4];
@@ -163,7 +163,7 @@ static void get_new_key_from_sha(struct ppp_mppe_state * state)
  * Perform the MPPE rekey algorithm, from RFC 3078, sec. 7.3.
  * Well, not what's written there, but rather what they meant.
  */
-static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
+static void mppe_rekey(struct ppp_mppe_state *state, int initial_key)
 {
 	struct scatterlist sg_in[1], sg_out[1];
 	struct blkcipher_desc desc = { .tfm = state->arc4 };
@@ -209,7 +209,6 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 	if (state = NULL)
 		goto out;
 
-
 	state->arc4 = crypto_alloc_blkcipher("ecb(arc4)", 0, CRYPTO_ALG_ASYNC);
 	if (IS_ERR(state->arc4)) {
 		state->arc4 = NULL;
@@ -243,15 +242,15 @@ static void *mppe_alloc(unsigned char *options, int optlen)
 
 	return (void *)state;
 
-	out_free:
-	    if (state->sha1_digest)
+out_free:
+	if (state->sha1_digest)
 		kfree(state->sha1_digest);
-	    if (state->sha1)
+	if (state->sha1)
 		crypto_free_hash(state->sha1);
-	    if (state->arc4)
+	if (state->arc4)
 		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
-	out:
+	kfree(state);
+out:
 	return NULL;
 }
 
@@ -260,26 +259,25 @@ static void *mppe_alloc(unsigned char *options, int optlen)
  */
 static void mppe_free(void *arg)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 	if (state) {
-	    if (state->sha1_digest)
-		kfree(state->sha1_digest);
-	    if (state->sha1)
-		crypto_free_hash(state->sha1);
-	    if (state->arc4)
-		crypto_free_blkcipher(state->arc4);
-	    kfree(state);
+		if (state->sha1_digest)
+			kfree(state->sha1_digest);
+		if (state->sha1)
+			crypto_free_hash(state->sha1);
+		if (state->arc4)
+			crypto_free_blkcipher(state->arc4);
+		kfree(state);
 	}
 }
 
 /*
  * Initialize (de)compressor state.
  */
-static int
-mppe_init(void *arg, unsigned char *options, int optlen, int unit, int debug,
-	  const char *debugstr)
+static int mppe_init(void *arg, unsigned char *options, int optlen, int unit,
+		     int debug, const char *debugstr)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 	unsigned char mppe_opts;
 
 	if (optlen != CILEN_MPPE ||
@@ -338,9 +336,8 @@ mppe_init(void *arg, unsigned char *options, int optlen, int unit, int debug,
 	return 1;
 }
 
-static int
-mppe_comp_init(void *arg, unsigned char *options, int optlen, int unit,
-	       int hdrlen, int debug)
+static int mppe_comp_init(void *arg, unsigned char *options, int optlen,
+			  int unit, int hdrlen, int debug)
 {
 	/* ARGSUSED */
 	return mppe_init(arg, options, optlen, unit, debug, "mppe_comp_init");
@@ -357,7 +354,7 @@ mppe_comp_init(void *arg, unsigned char *options, int optlen, int unit,
  */
 static void mppe_comp_reset(void *arg)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 
 	state->bits |= MPPE_BIT_FLUSHED;
 }
@@ -367,11 +364,10 @@ static void mppe_comp_reset(void *arg)
  * It's strange to call this a compressor, since the output is always
  * MPPE_OVHD + 2 bytes larger than the input.
  */
-static int
-mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
-	      int isize, int osize)
+static int mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
+			 int isize, int osize)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 	struct blkcipher_desc desc = { .tfm = state->arc4 };
 	int proto;
 	struct scatterlist sg_in[1], sg_out[1];
@@ -450,14 +446,13 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
  */
 static void mppe_comp_stats(void *arg, struct compstat *stats)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 
 	*stats = state->stats;
 }
 
-static int
-mppe_decomp_init(void *arg, unsigned char *options, int optlen, int unit,
-		 int hdrlen, int mru, int debug)
+static int mppe_decomp_init(void *arg, unsigned char *options, int optlen,
+			    int unit, int hdrlen, int mru, int debug)
 {
 	/* ARGSUSED */
 	return mppe_init(arg, options, optlen, unit, debug, "mppe_decomp_init");
@@ -483,11 +478,10 @@ static int mppe_cmp_ccount(unsigned int a, unsigned int b)
 /*
  * Decompress (decrypt) an MPPE packet.
  */
-static int
-mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
-		int osize)
+static int mppe_decompress(void *arg, unsigned char *ibuf, int isize,
+			   unsigned char *obuf, int osize)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 	struct blkcipher_desc desc = { .tfm = state->arc4 };
 	unsigned ccount;
 	int flushed = MPPE_BITS(ibuf) & MPPE_BIT_FLUSHED;
@@ -685,7 +679,7 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
  */
 static void mppe_incomp(void *arg, unsigned char *ibuf, int icnt)
 {
-	struct ppp_mppe_state *state = (struct ppp_mppe_state *) arg;
+	struct ppp_mppe_state *state = (struct ppp_mppe_state *)arg;
 
 	if (state->debug &&
 	    (PPP_PROTOCOL(ibuf) >= 0x0021 && PPP_PROTOCOL(ibuf) <= 0x00fa))
@@ -708,21 +702,21 @@ static void mppe_incomp(void *arg, unsigned char *ibuf, int icnt)
  */
 static struct compressor ppp_mppe = {
 	.compress_proto = CI_MPPE,
-	.comp_alloc     = mppe_alloc,
-	.comp_free      = mppe_free,
-	.comp_init      = mppe_comp_init,
-	.comp_reset     = mppe_comp_reset,
-	.compress       = mppe_compress,
-	.comp_stat      = mppe_comp_stats,
-	.decomp_alloc   = mppe_alloc,
-	.decomp_free    = mppe_free,
-	.decomp_init    = mppe_decomp_init,
-	.decomp_reset   = mppe_decomp_reset,
-	.decompress     = mppe_decompress,
-	.incomp         = mppe_incomp,
-	.decomp_stat    = mppe_comp_stats,
-	.owner          = THIS_MODULE,
-	.comp_extra     = MPPE_PAD,
+	.comp_alloc = mppe_alloc,
+	.comp_free = mppe_free,
+	.comp_init = mppe_comp_init,
+	.comp_reset = mppe_comp_reset,
+	.compress = mppe_compress,
+	.comp_stat = mppe_comp_stats,
+	.decomp_alloc = mppe_alloc,
+	.decomp_free = mppe_free,
+	.decomp_init = mppe_decomp_init,
+	.decomp_reset = mppe_decomp_reset,
+	.decompress = mppe_decompress,
+	.incomp = mppe_incomp,
+	.decomp_stat = mppe_comp_stats,
+	.owner = THIS_MODULE,
+	.comp_extra = MPPE_PAD,
 };
 
 /*
-- 
1.7.10.4

Re: [RFC PATCH v2 2/4] ppp_mppe: check coherency counter for out-of-order sequencing

[Eric Dumazet]

On Tue, 2013-05-21 at 20:18 +0200, Jorge Boncompte [DTI2] wrote:
> From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>

>  	if (!state->stateful) {
> +		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
> +			if (net_ratelimit())
> +				printk(KERN_WARNING "%s[%d]: Dropping out-of-order "
> +				       "packet with ccount %u, "
> +				       "expecting %u!\n", __func__,
> +				       state->unit, ccount, state->ccount);
> +			return DECOMP_DROPERROR;
> +		}

You did not address my review, thats too bad.

WARNING: networking block comments don't use an empty /* line, use /* Comment...
#107: FILE: drivers/net/ppp/ppp_mppe.c:483:
+
+/*

WARNING: Prefer netdev_warn(netdev, ... then dev_warn(dev, ... then pr_warn(...  to printk(KERN_WARNING ...
#117: FILE: drivers/net/ppp/ppp_mppe.c:564:
+				printk(KERN_WARNING "%s[%d]: Dropping out-of-order "

WARNING: quoted string split across lines
#118: FILE: drivers/net/ppp/ppp_mppe.c:565:
+				printk(KERN_WARNING "%s[%d]: Dropping out-of-order "
+				       "packet with ccount %u, "

Re: [RFC PATCH v2 2/4] ppp_mppe: check coherency counter for out-of-order sequencing

[Sergei Shtylyov]

Hello.

On 05/21/2013 10:18 PM, Jorge Boncompte [DTI2] wrote:

> From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
>
> While testing a L2TP tunnel without sequencing with MPPE encryption in
> stateless mode noticed that after a packet was reordered the encapsulated
> traffic session was stuck but testing against a Cisco gear did work.
>
>  From RFC3078 "MPPE expects packets to be delivered in sequence".
>
> The thing it's that the ppp_mppe module treats the reorder as if the
> coherency counter did wrap and rekeys all the "missing" packets.
>
> The link layer protocol should deliver the packets in order but at least
> with this patch in place the decryption process survives some packet reorder.
>
> Signed-off-by: Jorge Boncompte [DTI2] <jorge@dti2.net>
> ---
>   drivers/net/ppp/ppp_mppe.c |   23 ++++++++++++++++++++++-
>   1 file changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> index 9a1849a..97f4804 100644
> --- a/drivers/net/ppp/ppp_mppe.c
> +++ b/drivers/net/ppp/ppp_mppe.c
[...]
> @@ -547,6 +559,15 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
>   	 */
>   
>   	if (!state->stateful) {
> +		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
> +			if (net_ratelimit())
> +				printk(KERN_WARNING "%s[%d]: Dropping out-of-order "

     Why not pr_warn()?

WBR, Sergei

Re: [RFC PATCH v2 3/4] ppp_mppe: cleanup kernel log messages

[Joe Perches]

On Tue, 2013-05-21 at 20:18 +0200, Jorge Boncompte [DTI2] wrote:
> From: "Jorge Boncompte [DTI2]" <jorge@dti2.net>
> 
> - Consolidate log messages, print PPP unit number where available.
> - Changes error or warning messages to correct log level.
> - Use _ratelimited() functions for messages triggered by network packets.
[]
> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
[]
> @@ -178,7 +178,8 @@ static void mppe_rekey(struct ppp_mppe_state * state, int initial_key)
>  		setup_sg(sg_out, state->session_key, state->keylen);
>  		if (crypto_blkcipher_encrypt(&desc, sg_out, sg_in,
>  					     state->keylen) != 0) {
> -    		    printk(KERN_WARNING "mppe_rekey: cipher_encrypt failed\n");
> +			net_warn_ratelimited("%s[%d]: crypto_blkcipher_encrypt "
> +					     "failed\n", __func__, state->unit);

Please don't split formats across multiple lines.

			net_warn_ratelimited("%s[%d]: crypto_blkcipher_encrypt failed\n",
					     __func__, state->unit);

is just fine.

> @@ -314,8 +314,7 @@ mppe_init(void *arg, unsigned char *options, int optlen, int unit, int debug,
>  			sprintf(mkey + i * 2, "%02x", state->master_key[i]);
>  		for (i = 0; i < sizeof(state->session_key); i++)
>  			sprintf(skey + i * 2, "%02x", state->session_key[i]);
> -		printk(KERN_DEBUG
> -		       "%s[%d]: keys: master: %s initial session: %s\n",
> +		printk(KERN_DEBUG "%s[%d]: keys: master: %s initial session: %s\n",
>  		       debugstr, unit, mkey, skey);

this could probably use:
	%*ph, (int)sizeof(state->session_key), state->session_key

>  	}
>  
> @@ -387,9 +386,9 @@ mppe_compress(void *arg, unsigned char *ibuf, unsigned char *obuf,
[]
> +		net_err_ratelimited("%s[%d]: osize too small! "
> +				    "(have: %d need: %d)\n", __func__,
> +				    state->unit, osize, isize + MPPE_OVHD + 2);

another split format, please coalesce.

etc...