Message ID | 4B7E0879.4090504@canonical.com |
---|---|
State | Accepted |
Delegated to: | Andy Whitcroft |
Headers | show |
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
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