[PATCH] xt_recent: Fix buffer overflow

[Colin Ian King]

On Fri, 2010-02-19 at 12:42 +0200, Amit Kucheria wrote:
> On 10 Feb 18, Tim Gardner wrote:
> > If this looks right, then I'll send it upstream, and it should be a
> > pre-stable patch.
> > 
> > rtg
> > -- 
> > Tim Gardner tim.gardner at canonical.com
> 
> > From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
> > From: Tim Gardner <tim.gardner at canonical.com>
> > Date: Thu, 18 Feb 2010 20:04:51 -0700
> > Subject: [PATCH] xt_recent: Fix buffer overflow
> > 
> > e->index overflows e->stamps[] every ip_pkt_list_tot
> > packets.
> > 
> > Consider the case when ip_pkt_list_tot==1; the first packet received is stored
> > in e->stamps[0] and e->index is initialized to 1. The next received packet
> > timestamp is then stored at e->stamps[1] in recent_entry_update(),
> > a buffer overflow because the maximum e->stamps[] index is 0.
> > 
> > Signed-off-by: Tim Gardner <tim.gardner at canonical.com>
> > Cc: stable at kernel.org
> > ---
> >  net/netfilter/xt_recent.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
> > index fc70a49..1bb0d6c 100644
> > --- a/net/netfilter/xt_recent.c
> > +++ b/net/netfilter/xt_recent.c
> > @@ -173,10 +173,10 @@ recent_entry_init(struct recent_table *t, const union nf_inet_addr *addr,
> >  
> >  static void recent_entry_update(struct recent_table *t, struct recent_entry *e)
> >  {
> > +	e->index %= ip_pkt_list_tot;
> >  	e->stamps[e->index++] = jiffies;
> >  	if (e->index > e->nstamps)
> >  		e->nstamps = e->index;
> > -	e->index %= ip_pkt_list_tot;
> >  	list_move_tail(&e->lru_list, &t->lru_list);
> >  }
> >  
> > -- 
> > 1.6.2.4
> > 
> 
> This is a little more tricky I thought.
> 
> A brief look at the code tells me that e->stamps[] is supposed to store
> 'ip_pkt_list_tot' number of timestamps according to,
> 
> 	e = kmalloc(sizeof(*e) + sizeof(e->stamps[0]) * ip_pkt_list_tot,
> 			    GFP_ATOMIC);
> 
> And e->index is the index into the next slot to store a timestamp in. Is that
> correct?
> 
> So, won't the kmalloc above actually assign 2 'unsigned longs' when
> ip_pkt_list_tot == 1, one due to sizeof(*e), the other due to
> sizeof(e->stamps[0]) * ip_pkt_list_tot ? If so, the original code is doing
> the right thing - of not letting index overflow for the _next_ call to
> recent_entry_update().

Not sure I'm with you on that Amit.  The struct contains a zero sized
array stamps[] - this array is exactly zero bytes in size. So the
kmalloc allocates just ip_pkt_list_tot number of unsigned longs.  Hence
when ip_pkt_list_tot == 1, only 1 unsigned long is allocated.

I like stefan's recommendations of:
 
e->stamps[e->index] = jiffies;
e->index = (e->index + 1) % ip_pkt_list_tot;

Colin
> 
> /Amit
> 
> -- 
> ----------------------------------------------------------------------
> Amit Kucheria, Kernel Engineer || amit.kucheria at canonical.com
> ----------------------------------------------------------------------
>