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

[RFC PATCH 1/2] 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 2/2] 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 |   21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
index 9a1849a..0a10a6d 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"
@@ -469,6 +470,15 @@ 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 << 20) - (b << 20));
+}
+
+/*
  * Decompress (decrypt) an MPPE packet.
  */
 static int
@@ -547,6 +557,17 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
 	 */
 
 	if (!state->stateful) {
+		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
+			if (state->debug >= 7 && net_ratelimit())
+				printk(KERN_DEBUG
+				       "%s[%d:]: Dropping out-of-order packet, "
+				       "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

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

[Eric Dumazet]

On Mon, 2013-05-20 at 18:34 +0200, 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 |   21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
> index 9a1849a..0a10a6d 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"
> @@ -469,6 +470,15 @@ 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 << 20) - (b << 20));
> +}
> +

How was chosen this magical value ?

> +/*
>   * Decompress (decrypt) an MPPE packet.
>   */
>  static int
> @@ -547,6 +557,17 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
>  	 */
>  
>  	if (!state->stateful) {
> +		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
> +			if (state->debug >= 7 && net_ratelimit())
> +				printk(KERN_DEBUG
> +				       "%s[%d:]: Dropping out-of-order packet, "
> +				       "ccount %u expecting %u.\n",
> +				       __func__, state->unit, ccount,
> +				       state->ccount);
> +


net_dbg_ratelimited() ?

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

[Jorge Boncompte [DTI2]]

El 20/05/2013 19:32, Eric Dumazet escribió:
> On Mon, 2013-05-20 at 18:34 +0200, 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 |   21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
>> index 9a1849a..0a10a6d 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"
>> @@ -469,6 +470,15 @@ 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 << 20) - (b << 20));
>> +}
>> +
> 
> How was chosen this magical value ?

	The coherency count it's a 12-bit value. I'll add a define for it.

> 
>> +/*
>>   * Decompress (decrypt) an MPPE packet.
>>   */
>>  static int
>> @@ -547,6 +557,17 @@ mppe_decompress(void *arg, unsigned char *ibuf, int isize, unsigned char *obuf,
>>  	 */
>>  
>>  	if (!state->stateful) {
>> +		if (mppe_cmp_ccount(ccount, state->ccount) < 0) {
>> +			if (state->debug >= 7 && net_ratelimit())
>> +				printk(KERN_DEBUG
>> +				       "%s[%d:]: Dropping out-of-order packet, "
>> +				       "ccount %u expecting %u.\n",
>> +				       __func__, state->unit, ccount,
>> +				       state->ccount);
>> +
> 
> 
> net_dbg_ratelimited() ?

	I think it will be better if I prepare a third patch that cleanups the whole
file after.

-- 
===============================
Jorge Boncompte - Ingenieria y Gestion de RED
DTI2 - Desarrollo de la Tecnologia de las Comunicaciones
--------------------------------------------------------------
C/ Abogado Enriquez Barrios, 5   14004 CORDOBA (SPAIN)
Tlf: +34 957 761395 / FAX: +34 957 450380
===============================
- There is only so much duct tape you can put on something
  before it just becomes a giant ball of duct tape.
===============================