xt_recent: Fix buffer overflow

If this looks right, then I'll send it upstream, and it should be a
pre-stable patch.

rtg

Tim Gardner wrote:
> If this looks right, then I'll send it upstream, and it should be a
> pre-stable patch.
> 
> rtg
> 
Hm, assuming another call to that function happens while the first one has not
reached the restraining statement later. But if its that critical, I'd even feel
uneasy about doing an index++-
Your code now leaves e->index set to 1 after the call. Which might upset other
code. What about

e->stamps[e->index] = jiffies;
e->index = (e->index + 1) % ip_pkt_list_tot;

Stefan

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@canonical.com

> From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner@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@canonical.com>
> Cc: stable@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

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@canonical.com
> 
> > From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
> > From: Tim Gardner <tim.gardner@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@canonical.com>
> > Cc: stable@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@canonical.com
> ----------------------------------------------------------------------
>

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@canonical.com
>>> From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
>>> From: Tim Gardner <tim.gardner@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@canonical.com>
>>> Cc: stable@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

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@canonical.com
> >>> From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
> >>> From: Tim Gardner <tim.gardner@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@canonical.com>
> >>> Cc: stable@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

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@canonical.com
>>>>> From 478a6cbbd7646c78370da48677e99cc602076dd7 Mon Sep 17 00:00:00 2001
>>>>> From: Tim Gardner <tim.gardner@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@canonical.com>
>>>>> Cc: stable@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

Picked the version as applied upstream.

Applied to Lucid.

-apw