[PATCH] xt_recent: Fix buffer overflow

[Amit Kucheria]

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().

/Amit

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