ppp_mppe status for 2.6.x kernels

ppp_mppe status for 2.6.x kernels

[Matt Domsch]

Quick update on the state of ppp_mppe in kernel 2.6.x.  Works for me,
but more testers are needed, please test and post your results to
these lists.

Paul, I belive this addresses your concern about something being able
to send an unencrypted (uncompressed) packet after encryption has been
enabled.  This was done by adding a new field to struct compressor
called must_compress, and testing for it here:

        /* try to do packet compression */
        if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state != 0
            && proto != PPP_LCP && proto != PPP_CCP) {
+               if (!(ppp->flags & SC_CCP_UP) && ppp->xcomp->must_compress) {
+                       printk(KERN_ERR "ppp: compression required but down - pkt dropped.\n");
                        goto drop;
                }


The code is here:

BK:
http://mdomsch.bkbits.net/linux-2.6-mppe

Patches:
http://domsch.com/linux/pptp/2.6.9-rc4/linux-2.6.9-rc4-ppp_mppe.patch
http://domsch.com/linux/pptp/2.6.9-rc4/linux-2.6.9-rc4-ppp_mppe.patch.sign

 drivers/net/Kconfig       |    6
 drivers/net/Makefile      |    1
 drivers/net/ppp_generic.c |   76 +++-
 drivers/net/ppp_mppe.c    |  721 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/net/ppp_mppe.h    |   87 +++++
 include/linux/ppp-comp.h  |   14
 6 files changed, 882 insertions, 23 deletions

through these ChangeSets:

<Matt_Domsch@dell.com> (04/10/12 1.2166)
   ppp_mppe: make SHA pad bytes once per driver rather than per-connection.

<Matt_Domsch@dell.com> (04/08/30 1.1803.16.4)
   ppp_mppe: use setup_sg() in get_hew_key_from_sha(), bump version

<mole@quadra.ru> (04/08/30 1.1803.16.3)
   ppp_mppe and ppp_generic.c: bug fixes
   
   From: Oleg Makarenko [mole@quadra.ru]
   Sent: Friday, July 30, 2004 2:33 PM
   
                                                                                                       
   1. setup_sg(). Do you really need to split the data this way? The
   documentation on crypto api and scatterlists is not very helpful so I
   could be wrong here but in my reading  of crypto/digest.c/update() (for
   ex)  you may just have a single sg[0] even if the data doesn't fit into
   a single page.  All pages just need to be contiguous. It probably
   doesn't hirt but is it really needed?  I have removed this split from
   your patch just to test it. Seems to work fine.
                                                                                                       
   2.  For some reason you can not use non GFP_KERNEL memory and scatter
   lists or at least mix them in crypto_digest().  That is why sha_pad is
   now in struct state {}.
                                                                                                       
   3.  In get_new_key_from_sha() the code like
                                                                                                       
      crypto_digest_update(state->sha1, sg, state->keylen)
                                                                                                       
   looks suspicious as the last parameter (in my reading of digest.c)
   should be the number of elements in sg[] array not the data length. That
   seems to be the reason for my kernel panics. With your setup_sg() the
   last parameter should be 1 or 2. Or may be you could replace all four
   digest_update calls with a single call with properly initilaized sg[4]
   (to make some use from scatterlist) . See the modified patch for details.
   
                                                                                                       
   4. in ppp_generic.c/pad_compress_skb() the following code:
                                                                                                       
          } else if (len = 0) {
                  /* didn't compress, or CCP not up yet */
                  kfree_skb(new_skb);
                  new_skb = NULL;
                  ...
           return new_skb;
                                                                                                       
   also looks suspicious as later I read
                                                                                                       
                  skb = pad_compress_skb(ppp, skb);
                  if (!skb)
                          goto drop;
                                                                                                       
   I think you don't want to drop packets with len = 0.  At least the
   previous mppe enabled ppp_generic.c code don't do that.  I've changed
   new_skb = NULL above to new_skb = skb, see the patch. IPCP can not be
   established without this modification.
   

<Matt_Domsch@dell.com> (04/07/20 1.1803.16.2)
   PPP: Add drivers/net/ppp_mppe.c, Makefile and Kconfig entries
   
   Microsoft Point-to-Point Tunnelling Protocol support
   utilizes ppp_generic and kernel crypto routines.

<Matt_Domsch@dell.com> (04/07/20 1.1803.16.1)
   PPP: add fields to struct compressor, use them in ppp-generic.c
   
   Add fields to struct compressor to allocate extra skb space
   for compression/decompression, and a flag to drop packets
   if CCP is down but required by the compression (encryption) alg.



I'd like to see this included after 2.6.9 is released, if possible,
but want to be sure everyone's concerns have been addressed.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

Re: [pptp-devel] ppp_mppe status for 2.6.x kernels

[Matt Domsch]

On Tue, Oct 12, 2004 at 05:05:28PM -0500, Matt Domsch wrote:
> Quick update on the state of ppp_mppe in kernel 2.6.x.  Works for me,
> but more testers are needed, please test and post your results to
> these lists.
>
> BK:
> http://mdomsch.bkbits.net/linux-2.6-mppe

It would help if I actually freed the newly allocated sha_pad
struct...  I pushed this into the above BK tree.

Oleg, Frank, I'm going to need to get a "Signed-of-by:" line for each
of you for final patch submission.  See
linux/Documentation/SubmittingPatches for the format and what you're
agreeing to.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

ChangeSet@1.2167, 2004-10-12 18:00:10-05:00, Matt_Domsch@dell.com
  ppp_mppe: free sha_pad properly
  
  Signed-off-by: Matt Domsch <Matt_Domsch@dell.com>


 ppp_mppe.c |    3 +++
 1 files changed, 3 insertions


diff -Nru a/drivers/net/ppp_mppe.c b/drivers/net/ppp_mppe.c
--- a/drivers/net/ppp_mppe.c	2004-10-12 21:44:58 -05:00
+++ b/drivers/net/ppp_mppe.c	2004-10-12 21:44:58 -05:00
@@ -708,6 +708,8 @@
 
 	if (answer = 0)
 		printk(KERN_INFO "PPP MPPE Compression module registered\n");
+	else
+		kfree(sha_pad);
 
 	return answer;
 }
@@ -715,6 +717,7 @@
 static void __exit ppp_mppe_cleanup(void)
 {
 	ppp_unregister_compressor(&ppp_mppe);
+	kfree(sha_pad);
 }
 
 module_init(ppp_mppe_init);

Re: ppp_mppe status for 2.6.x kernels

[Michael Tokarev]

Matt Domsch wrote:
> Quick update on the state of ppp_mppe in kernel 2.6.x.  Works for me,
> but more testers are needed, please test and post your results to
> these lists.
> 
> Paul, I belive this addresses your concern about something being able
> to send an unencrypted (uncompressed) packet after encryption has been
> enabled.  This was done by adding a new field to struct compressor
> called must_compress, and testing for it here:
> 
>         /* try to do packet compression */
>         if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state != 0
>             && proto != PPP_LCP && proto != PPP_CCP) {
> +               if (!(ppp->flags & SC_CCP_UP) && ppp->xcomp->must_compress) {
> +                       printk(KERN_ERR "ppp: compression required but down - pkt dropped.\n");
>                         goto drop;
>                 }
> 

Please don't do that -- don't flood syslog.  See how similar
things are done in other net/ code (eg, using net_ratelimit()).



> The code is here:
> 
> BK:
> http://mdomsch.bkbits.net/linux-2.6-mppe
> 
> Patches:
> http://domsch.com/linux/pptp/2.6.9-rc4/linux-2.6.9-rc4-ppp_mppe.patch
> http://domsch.com/linux/pptp/2.6.9-rc4/linux-2.6.9-rc4-ppp_mppe.patch.sign
> 
>  drivers/net/Kconfig       |    6
>  drivers/net/Makefile      |    1
>  drivers/net/ppp_generic.c |   76 +++-
>  drivers/net/ppp_mppe.c    |  721 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/net/ppp_mppe.h    |   87 +++++
>  include/linux/ppp-comp.h  |   14
>  6 files changed, 882 insertions, 23 deletions
> 
> through these ChangeSets:
> 
> <Matt_Domsch@dell.com> (04/10/12 1.2166)
>    ppp_mppe: make SHA pad bytes once per driver rather than per-connection.
> 
> <Matt_Domsch@dell.com> (04/08/30 1.1803.16.4)
>    ppp_mppe: use setup_sg() in get_hew_key_from_sha(), bump version
> 
> <mole@quadra.ru> (04/08/30 1.1803.16.3)
>    ppp_mppe and ppp_generic.c: bug fixes
>    
>    From: Oleg Makarenko [mole@quadra.ru]
>    Sent: Friday, July 30, 2004 2:33 PM
>    
>                                                                                                        
>    1. setup_sg(). Do you really need to split the data this way? The
>    documentation on crypto api and scatterlists is not very helpful so I
>    could be wrong here but in my reading  of crypto/digest.c/update() (for
>    ex)  you may just have a single sg[0] even if the data doesn't fit into
>    a single page.  All pages just need to be contiguous. It probably
>    doesn't hirt but is it really needed?  I have removed this split from
>    your patch just to test it. Seems to work fine.
>                                                                                                        
>    2.  For some reason you can not use non GFP_KERNEL memory and scatter
>    lists or at least mix them in crypto_digest().  That is why sha_pad is
>    now in struct state {}.
>                                                                                                        
>    3.  In get_new_key_from_sha() the code like
>                                                                                                        
>       crypto_digest_update(state->sha1, sg, state->keylen)
>                                                                                                        
>    looks suspicious as the last parameter (in my reading of digest.c)
>    should be the number of elements in sg[] array not the data length. That
>    seems to be the reason for my kernel panics. With your setup_sg() the
>    last parameter should be 1 or 2. Or may be you could replace all four
>    digest_update calls with a single call with properly initilaized sg[4]
>    (to make some use from scatterlist) . See the modified patch for details.
>    
>                                                                                                        
>    4. in ppp_generic.c/pad_compress_skb() the following code:
>                                                                                                        
>           } else if (len = 0) {
>                   /* didn't compress, or CCP not up yet */
>                   kfree_skb(new_skb);
>                   new_skb = NULL;
>                   ...
>            return new_skb;
>                                                                                                        
>    also looks suspicious as later I read
>                                                                                                        
>                   skb = pad_compress_skb(ppp, skb);
>                   if (!skb)
>                           goto drop;
>                                                                                                        
>    I think you don't want to drop packets with len = 0.  At least the
>    previous mppe enabled ppp_generic.c code don't do that.  I've changed
>    new_skb = NULL above to new_skb = skb, see the patch. IPCP can not be
>    established without this modification.
>    
> 
> <Matt_Domsch@dell.com> (04/07/20 1.1803.16.2)
>    PPP: Add drivers/net/ppp_mppe.c, Makefile and Kconfig entries
>    
>    Microsoft Point-to-Point Tunnelling Protocol support
>    utilizes ppp_generic and kernel crypto routines.
> 
> <Matt_Domsch@dell.com> (04/07/20 1.1803.16.1)
>    PPP: add fields to struct compressor, use them in ppp-generic.c
>    
>    Add fields to struct compressor to allocate extra skb space
>    for compression/decompression, and a flag to drop packets
>    if CCP is down but required by the compression (encryption) alg.
> 
> 
> 
> I'd like to see this included after 2.6.9 is released, if possible,
> but want to be sure everyone's concerns have been addressed.
> 
> Thanks,
> Matt
> 

Re: [pptp-devel] Re: ppp_mppe status for 2.6.x kernels

[Matt Domsch]

On Wed, Oct 13, 2004 at 02:47:16PM +0400, Michael Tokarev wrote:
> >+                       printk(KERN_ERR "ppp: compression required but 
> >down - pkt dropped.\n");
>
> Please don't do that -- don't flood syslog.  See how similar
> things are done in other net/ code (eg, using net_ratelimit()).

Good point.  I've added net_ratelimit()s to the printks that my patch
touches.  There are a few others in ppp_generic.c which might benefit
from the same treatment, but I'd rather not do those in conjunction
with the changes necessary for ppp_mppe inclusion.

How's this look?

-- 
Matt Domsch
Sr. Software Engineer, Lead Engineer
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com

=== drivers/net/ppp_generic.c 1.50 vs edited ==--- 1.50/drivers/net/ppp_generic.c	2004-10-12 11:10:48 -05:00
+++ edited/drivers/net/ppp_generic.c	2004-10-13 09:36:22 -05:00
@@ -1017,7 +1017,8 @@
 	int compressor_skb_size = ppp->dev->mtu + ppp->xcomp->comp_skb_extra_space + PPP_HDRLEN;
 	new_skb = alloc_skb(new_skb_size, GFP_ATOMIC);
 	if (!new_skb) {
-		printk(KERN_ERR "PPP: no memory (comp pkt)\n");
+		if (net_ratelimit())
+			printk(KERN_ERR "PPP: no memory (comp pkt)\n");
 		return NULL;
 	}
 	if (ppp->dev->hard_header_len > PPP_HDRLEN)
@@ -1046,7 +1047,8 @@
 		 * the compress_proto because MPPE and MPPC share
 		 * the same number.
 		 */
-		printk(KERN_ERR "ppp: compressor dropped pkt\n");
+		if (net_ratelimit())
+			printk(KERN_ERR "ppp: compressor dropped pkt\n");
 		kfree_skb(new_skb);
 		new_skb = NULL;
 	}
@@ -1140,7 +1142,8 @@
 	if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state != 0
 	    && proto != PPP_LCP && proto != PPP_CCP) {
 		if (!(ppp->flags & SC_CCP_UP) && ppp->xcomp->must_compress) {
-			printk(KERN_ERR "ppp: compression required but down - pkt dropped.\n");
+			if (net_ratelimit())
+				printk(KERN_ERR "ppp: compression required but down - pkt dropped.\n");
 			goto drop;
 		}
 		skb = pad_compress_skb(ppp, skb);

Re: ppp_mppe status for 2.6.x kernels

[Paul Mackerras]

Matt Domsch writes:

> Paul, I belive this addresses your concern about something being able
> to send an unencrypted (uncompressed) packet after encryption has been
> enabled.  This was done by adding a new field to struct compressor
> called must_compress, and testing for it here:
> 
>         /* try to do packet compression */
>         if ((ppp->xstate & SC_COMP_RUN) && ppp->xc_state != 0
>             && proto != PPP_LCP && proto != PPP_CCP) {
> +               if (!(ppp->flags & SC_CCP_UP) && ppp->xcomp->must_compress) {
> +                       printk(KERN_ERR "ppp: compression required but down - pkt dropped.\n");
>                         goto drop;
>                 }

That still only does anything while SC_COMP_RUN is set in
ppp->xstate.  If CCP goes down, SC_COMP_RUN gets cleared and we are no
better off than before.  The flag needs to be in ppp->flags and needs
to be set by pppd before we start negotiating.

Paul.