[PATCH] xt_recent: Fix buffer overflow
Tim Gardner
tim.gardner at canonical.com
Fri Feb 19 14:35:09 UTC 2010
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.
rtg
--
Tim Gardner tim.gardner at canonical.com
More information about the kernel-team
mailing list