[PATCH] xt_recent: Fix buffer overflow

Tim Gardner tim.gardner at canonical.com
Fri Feb 19 18:11:03 GMT 2010


Andy Whitcroft wrote:
> On Fri, Feb 19, 2010 at 07:35:09AM -0700, Tim Gardner wrote:
>> Colin Ian King wrote:
>>> 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
>>>>
>> I wrote an example program last night while I was puzzling over this patch:
>>
>> #include <stdio.h>
>> struct s {
>> 	int length;
>> 	int array[0];
>> };
>> int main(int argc,char *argv[])
>> {
>> 	printf("sizeof(struct s) == %lu\n",sizeof(struct s));
>> }
>>
>> sizeof(struct s) == 4
>>
>> I don't think Stefan's patch will do the right thing in
>> recent_seq_show() which assumes e->index is out of bounds. I know, its
>> not very robust code, but I'm only gonna change the overflow.
> 
> Note that the size is only the int at the top, there is (as one might
> expect) no size associated with the array[0] element.
> 
>>>> 	e = kmalloc(sizeof(*e) + sizeof(e->stamps[0]) * ip_pkt_list_tot,
>>>> 			    GFP_ATOMIC);
> 
> Now if that really is the allocation it seems wrong, though too big.  I
> would have expected it to be 
> 
> 	sizeof(*e) + (sizeof(e->stamps[0] * ip_pkt_list_tot))
> 
> -apw

I think the original allocation is correct. For example:

printf("sizeof(s.array[0]) == %lu\n", sizeof(((struct s *)0)->array[0]));

sizeof(s.array[0]) == 4

rtg
-- 
Tim Gardner tim.gardner at canonical.com



More information about the kernel-team mailing list