Re: [Fwd: [patch 02/15] ppp_mppe: add PPP MPPE encryption module]

[Matt Domsch <Matt_Domsch-8PEkshWhKlo@public.gmane.org>]

On Fri, Jul 08, 2005 at 09:42:21PM +1000, Paul Mackerras wrote:
> Some comments on the MPPE kernel patch (sorry it's taken me so long):

Likewise, my apologies for the delay in implementing your
suggestions.  I do really appreciate your feedback.

 
> > +static inline struct sk_buff *
> > +pad_compress_skb(struct ppp *ppp, struct sk_buff *skb)
> > +{
> > +	struct sk_buff *new_skb;
> > +	int len;
> > +	int new_skb_size = ppp->dev->mtu + ppp->xcomp->comp_skb_extra_space + ppp->dev->hard_header_len;
> > +	int compressor_skb_size = ppp->dev->mtu + ppp->xcomp->comp_skb_extra_space + PPP_HDRLEN;
> 
> This would be nicer if you broke these lines each into two lines.  It
> would help if you chose shorter names for comp_skb_extra_space and
> decomp_skb_extra_space (just comp_extra, for instance).  And why do we
> need decomp_skb_extra_space anyway?  We expect the decompressor to
> expand things; if they shrink (as they will with MPPE) we don't have
> to do anything special.

Done.
 
> > +	} else {
> > +		/*
> > +		 * (len < 0)
> > +		 * MPPE requires that we do not send unencrypted
> > +		 * frames.  The compressor will return -1 if we
> > +		 * should drop the frame.  We cannot simply test
> > +		 * the compress_proto because MPPE and MPPC share
> > +		 * the same number.
> > +		 */
> > +		if (net_ratelimit())
> > +			printk(KERN_ERR "ppp: compressor dropped pkt\n");
> > +		kfree_skb(new_skb);
> > +		new_skb = NULL;
> 
> I think we just leaked the original skb in this case.

Good catch, fixed.

 
> > @@ -1683,7 +1716,7 @@ ppp_decompress_frame(struct ppp *ppp, st
> >  		goto err;
> >  
> >  	if (proto == PPP_COMP) {
> > -		ns = dev_alloc_skb(ppp->mru + PPP_HDRLEN);
> > +		ns = dev_alloc_skb(ppp->mru + ppp->rcomp->decomp_skb_extra_space + PPP_HDRLEN);
> >  		if (ns == 0) {
> >  			printk(KERN_ERR "ppp_decompress_frame: no memory\n");
> >  			goto err;
> 
> I don't see where you make sure that you drop any unencrypted received
> frames.

Fixed.

On Fri, Jul 08, 2005 at 10:57:15PM +1000, Paul Mackerras wrote:
> To follow up on my comments on the mppe patch, it still misses the
> most important thing, which is to make sure you don't send unencrypted
> data if CCP should go down.  A received CCP TermReq or TermAck will
> clear the SC_DECOMP_RUN flag and the code will then ignore the
> xcomp->must_compress flag.
> 
> I really think there should be another flag bit set by pppd to say
> "must compress" rather than relying on the compressor telling you
> that.

I've added a new flag SC_MUST_COMPRESS, which pppd sets on CCP UP with
MPPE enabled, and never clears it.  The new kernel code checks for
this flag at both compression and decompression time, and drops the
packets if compression or decompression fails.

pppd also drops LCP (thus the connection) if CCP goes down or gets
renegotiated, as tested with kill -USR2 $pid-of-pppd.  So a user can't
force renegotiation without losing their connection.

Kernel and pppd patches will follow in separate messages.  Lightly
tested, additional review much appreciated.

Thanks,
Matt

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


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf