* [2/2] osf: fixed /proc reading bug
@ 2004-08-21 21:03 Evgeniy Polyakov
2004-08-21 22:30 ` Patrick McHardy
0 siblings, 1 reply; 11+ messages in thread
From: Evgeniy Polyakov @ 2004-08-21 21:03 UTC (permalink / raw)
To: Harald Welte; +Cc: netfilter-devel
[-- Attachment #1: Type: text/plain, Size: 193 bytes --]
Fixed buffer overflow when reading rules from /proc file.
Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
Evgeniy Polyakov ( s0mbre )
Only failure makes us experts. -- Theo de Raadt
[-- Attachment #2: ipt_osf.diff.1 --]
[-- Type: application/octet-stream, Size: 1217 bytes --]
--- netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400
+++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400
@@ -182,7 +185,6 @@
optsize = tcp->doff*4 - sizeof(struct tcphdr);
}
-
/* Actually we can create hash/table of all genres and search
* only in appropriate part, but here is initial variant,
* so will use slow path.
@@ -601,9 +603,10 @@
{
struct list_head *ent;
struct osf_finger *f = NULL;
- int i;
+ int i, __count, err;
*eof = 1;
+ __count = count;
count = 0;
read_lock_bh(&osf_lock);
@@ -613,10 +616,13 @@
log("%s [%s]", f->genre, f->details);
- count += sprintf(buf+count, "%s - %s[%s] : %s",
+ err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s",
f->genre, f->version,
f->subtype, f->details);
-
+ if (err < 0)
+ break;
+ else
+ count += err;
if (f->opt_num)
{
loga(" OPT: ");
@@ -630,7 +636,11 @@
}
}
loga("\n");
- count += sprintf(buf+count, "\n");
+ err = snprintf(buf+count, __count-count, "\n");
+ if (err < 0)
+ break;
+ else
+ count += err;
}
read_unlock_bh(&osf_lock);
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [2/2] osf: fixed /proc reading bug 2004-08-21 21:03 [2/2] osf: fixed /proc reading bug Evgeniy Polyakov @ 2004-08-21 22:30 ` Patrick McHardy 2004-08-21 23:48 ` Henrik Nordstrom 0 siblings, 1 reply; 11+ messages in thread From: Patrick McHardy @ 2004-08-21 22:30 UTC (permalink / raw) To: johnpol; +Cc: Harald Welte, netfilter-devel Evgeniy Polyakov wrote: >Fixed buffer overflow when reading rules from /proc file. > How is this supposed to fix it ? log("%s [%s]", f->genre, f->details); - count += sprintf(buf+count, "%s - %s[%s] : %s", + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", f->genre, f->version, f->subtype, f->details); - + if (err < 0) + break; + else + count += err; if (f->opt_num) { loga(" OPT: "); snprintf returns the number of characters written if n <= limit, otherwise the number of characters that would have been generated for the given input, but never < 0. You can also use vscnprintf to get the real number of bytes written. Regards Patrick ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-21 22:30 ` Patrick McHardy @ 2004-08-21 23:48 ` Henrik Nordstrom 2004-08-22 0:15 ` Patrick McHardy 0 siblings, 1 reply; 11+ messages in thread From: Henrik Nordstrom @ 2004-08-21 23:48 UTC (permalink / raw) To: Patrick McHardy; +Cc: johnpol, Harald Welte, netfilter-devel On Sun, 22 Aug 2004, Patrick McHardy wrote: > How is this supposed to fix it ? The switch from sprintf to snprintf is a good start. > log("%s [%s]", f->genre, f->details); > > - count += sprintf(buf+count, "%s - %s[%s] : %s", > + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", > f->genre, f->version, > f->subtype, f->details); > - > + if (err < 0) This should read something like follows I think: if (err < 0 || err > __count-count) > + break; > + else > + count += err; > if (f->opt_num) > { > loga(" OPT: "); > > snprintf returns the number of characters written if n <= limit, otherwise > the number of characters that would have been generated for the given input, > but never < 0. You can also use vscnprintf to get the real number of bytes > written. There may still be systems around with old snprintf implementations returning -1 on overflow unless this is kernel code. Regards Henrik ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-21 23:48 ` Henrik Nordstrom @ 2004-08-22 0:15 ` Patrick McHardy 2004-08-23 8:57 ` Evgeniy Polyakov 0 siblings, 1 reply; 11+ messages in thread From: Patrick McHardy @ 2004-08-22 0:15 UTC (permalink / raw) To: Henrik Nordstrom; +Cc: johnpol, Harald Welte, netfilter-devel Henrik Nordstrom wrote: > On Sun, 22 Aug 2004, Patrick McHardy wrote: > >> log("%s [%s]", f->genre, f->details); >> >> - count += sprintf(buf+count, "%s - %s[%s] : %s", >> + err = snprintf(buf+count, __count-count, "%s - %s[%s] >> : %s", >> f->genre, f->version, >> f->subtype, f->details); >> - >> + if (err < 0) > > > This should read something like follows I think: > > if (err < 0 || err > __count-count) > >> + break; >> + else >> + count += err; >> if (f->opt_num) >> { >> loga(" OPT: "); > Yes, but the check for < 0 is not needed and it should read >= __count - count to avoid useless zero-byte snprintfs. Evgeniy, can you please send a new patch ? >> >> snprintf returns the number of characters written if n <= limit, >> otherwise >> the number of characters that would have been generated for the given >> input, >> but never < 0. You can also use vscnprintf to get the real number of >> bytes >> written. > > > There may still be systems around with old snprintf implementations > returning -1 on overflow unless this is kernel code. It is. The *nprintf behaviour really is surprising (and annoying for chaining multiple snprintfs), given the behaviour of the *printf functions. OTOH, there is no other way to distinguish "n bytes written" with "truncated to n bytes" and returning -1 is even worse. Regards Patrick ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-22 0:15 ` Patrick McHardy @ 2004-08-23 8:57 ` Evgeniy Polyakov 2004-08-23 9:55 ` Patrick McHardy 0 siblings, 1 reply; 11+ messages in thread From: Evgeniy Polyakov @ 2004-08-23 8:57 UTC (permalink / raw) To: Patrick McHardy; +Cc: Henrik Nordstrom, Harald Welte, netfilter-devel [-- Attachment #1.1: Type: text/plain, Size: 1399 bytes --] On Sun, 2004-08-22 at 04:15, Patrick McHardy wrote: > Henrik Nordstrom wrote: > > > On Sun, 22 Aug 2004, Patrick McHardy wrote: > > > >> log("%s [%s]", f->genre, f->details); > >> > >> - count += sprintf(buf+count, "%s - %s[%s] : %s", > >> + err = snprintf(buf+count, __count-count, "%s - %s[%s] > >> : %s", > >> f->genre, f->version, > >> f->subtype, f->details); > >> - > >> + if (err < 0) > > > > > > This should read something like follows I think: > > > > if (err < 0 || err > __count-count) > > > >> + break; > >> + else > >> + count += err; > >> if (f->opt_num) > >> { > >> loga(" OPT: "); > > > > Yes, but the check for < 0 is not needed and it should read >= __count - > count > to avoid useless zero-byte snprintfs. Evgeniy, can you please send a new > patch ? Sure. It simply checks if return value from snprintf is 0 and breaks, otherwise it proceeds. ipt_osf.diff.1 - patch for 2.6 ipt_osf.diff.1.24 - patch for 2.4 Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru> > Regards > Patrick -- Evgeniy Polyakov ( s0mbre ) Crash is better than data corruption. -- Art Grabowski [-- Attachment #1.2: ipt_osf.diff.1 --] [-- Type: text/plain, Size: 1270 bytes --] --- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 +++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 @@ -182,7 +185,6 @@ optsize = tcp->doff*4 - sizeof(struct tcphdr); } - /* Actually we can create hash/table of all genres and search * only in appropriate part, but here is initial variant, * so will use slow path. @@ -601,9 +603,10 @@ { struct list_head *ent; struct osf_finger *f = NULL; - int i; + int i, __count, err; *eof = 1; + __count = count; count = 0; read_lock_bh(&osf_lock); @@ -613,10 +616,13 @@ log("%s [%s]", f->genre, f->details); - count += sprintf(buf+count, "%s - %s[%s] : %s", + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", f->genre, f->version, f->subtype, f->details); - + if (err == 0) + break; + else + count += err; if (f->opt_num) { loga(" OPT: "); @@ -630,7 +636,11 @@ } } loga("\n"); - count += sprintf(buf+count, "\n"); + err = snprintf(buf+count, __count-count, "\n"); + if (err == 0) + break; + else + count += err; } read_unlock_bh(&osf_lock); [-- Attachment #1.3: ipt_osf.diff.1.24 --] [-- Type: text/plain, Size: 1270 bytes --] --- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 +++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 @@ -182,7 +185,6 @@ optsize = tcp->doff*4 - sizeof(struct tcphdr); } - /* Actually we can create hash/table of all genres and search * only in appropriate part, but here is initial variant, * so will use slow path. @@ -601,9 +603,10 @@ { struct list_head *ent; struct osf_finger *f = NULL; - int i; + int i, __count, err; *eof = 1; + __count = count; count = 0; read_lock_bh(&osf_lock); @@ -613,10 +616,13 @@ log("%s [%s]", f->genre, f->details); - count += sprintf(buf+count, "%s - %s[%s] : %s", + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", f->genre, f->version, f->subtype, f->details); - + if (err == 0) + break; + else + count += err; if (f->opt_num) { loga(" OPT: "); @@ -630,7 +636,11 @@ } } loga("\n"); - count += sprintf(buf+count, "\n"); + err = snprintf(buf+count, __count-count, "\n"); + if (err == 0) + break; + else + count += err; } read_unlock_bh(&osf_lock); [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-23 8:57 ` Evgeniy Polyakov @ 2004-08-23 9:55 ` Patrick McHardy 2004-08-23 10:30 ` Evgeniy Polyakov 0 siblings, 1 reply; 11+ messages in thread From: Patrick McHardy @ 2004-08-23 9:55 UTC (permalink / raw) To: johnpol; +Cc: Henrik Nordstrom, Harald Welte, netfilter-devel Evgeniy Polyakov wrote: >It simply checks if return value from snprintf is 0 and breaks, >otherwise it proceeds. > Still broken. snprintf returns a value > n if it truncated to n bytes. See my last mail again. BTW, did the overflow actually cause problems ? proc has an extra k of space just for overflows .. Regards Patrick >ipt_osf.diff.1 - patch for 2.6 >ipt_osf.diff.1.24 - patch for 2.4 > >Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru> > > > >>Regards >>Patrick >> >> >>------------------------------------------------------------------------ >> >>--- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 >>+++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 >>@@ -182,7 +185,6 @@ >> optsize = tcp->doff*4 - sizeof(struct tcphdr); >> } >> >>- >> /* Actually we can create hash/table of all genres and search >> * only in appropriate part, but here is initial variant, >> * so will use slow path. >>@@ -601,9 +603,10 @@ >> { >> struct list_head *ent; >> struct osf_finger *f = NULL; >>- int i; >>+ int i, __count, err; >> >> *eof = 1; >>+ __count = count; >> count = 0; >> >> read_lock_bh(&osf_lock); >>@@ -613,10 +616,13 @@ >> >> log("%s [%s]", f->genre, f->details); >> >>- count += sprintf(buf+count, "%s - %s[%s] : %s", >>+ err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", >> f->genre, f->version, >> f->subtype, f->details); >>- >>+ if (err == 0) >>+ break; >>+ else >>+ count += err; >> if (f->opt_num) >> { >> loga(" OPT: "); >>@@ -630,7 +636,11 @@ >> } >> } >> loga("\n"); >>- count += sprintf(buf+count, "\n"); >>+ err = snprintf(buf+count, __count-count, "\n"); >>+ if (err == 0) >>+ break; >>+ else >>+ count += err; >> } >> read_unlock_bh(&osf_lock); >> >> >> >>------------------------------------------------------------------------ >> >>--- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 >>+++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 >>@@ -182,7 +185,6 @@ >> optsize = tcp->doff*4 - sizeof(struct tcphdr); >> } >> >>- >> /* Actually we can create hash/table of all genres and search >> * only in appropriate part, but here is initial variant, >> * so will use slow path. >>@@ -601,9 +603,10 @@ >> { >> struct list_head *ent; >> struct osf_finger *f = NULL; >>- int i; >>+ int i, __count, err; >> >> *eof = 1; >>+ __count = count; >> count = 0; >> >> read_lock_bh(&osf_lock); >>@@ -613,10 +616,13 @@ >> >> log("%s [%s]", f->genre, f->details); >> >>- count += sprintf(buf+count, "%s - %s[%s] : %s", >>+ err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", >> f->genre, f->version, >> f->subtype, f->details); >>- >>+ if (err == 0) >>+ break; >>+ else >>+ count += err; >> if (f->opt_num) >> { >> loga(" OPT: "); >>@@ -630,7 +636,11 @@ >> } >> } >> loga("\n"); >>- count += sprintf(buf+count, "\n"); >>+ err = snprintf(buf+count, __count-count, "\n"); >>+ if (err == 0) >>+ break; >>+ else >>+ count += err; >> } >> read_unlock_bh(&osf_lock); >> >> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-23 9:55 ` Patrick McHardy @ 2004-08-23 10:30 ` Evgeniy Polyakov 2004-08-23 10:38 ` Henrik Nordstrom 2004-08-23 10:39 ` Evgeniy Polyakov 0 siblings, 2 replies; 11+ messages in thread From: Evgeniy Polyakov @ 2004-08-23 10:30 UTC (permalink / raw) To: Patrick McHardy; +Cc: Henrik Nordstrom, Harald Welte, netfilter-devel [-- Attachment #1: Type: text/plain, Size: 4119 bytes --] On Mon, 2004-08-23 at 13:55, Patrick McHardy wrote: > Evgeniy Polyakov wrote: > > >It simply checks if return value from snprintf is 0 and breaks, > >otherwise it proceeds. > > > Still broken. snprintf returns a value > n if it truncated to n bytes. > See my last mail again. BTW, did the overflow actually cause problems ? > proc has an extra k of space just for overflows .. If it truncates than we have [avoided] overflow and definetely will not write anything after it(except zero-lengh snprintf) since __count-count == 0 there. Do you mean following: list_for_each() { snprintf(); if (count > __count) break; } > > Regards > Patrick > > >ipt_osf.diff.1 - patch for 2.6 > >ipt_osf.diff.1.24 - patch for 2.4 > > > >Signed-off-by: Evgeniy Polyakov <johnpol@2ka.mipt.ru> > > > > > > > >>Regards > >>Patrick > >> > >> > >>------------------------------------------------------------------------ > >> > >>--- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 > >>+++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 > >>@@ -182,7 +185,6 @@ > >> optsize = tcp->doff*4 - sizeof(struct tcphdr); > >> } > >> > >>- > >> /* Actually we can create hash/table of all genres and search > >> * only in appropriate part, but here is initial variant, > >> * so will use slow path. > >>@@ -601,9 +603,10 @@ > >> { > >> struct list_head *ent; > >> struct osf_finger *f = NULL; > >>- int i; > >>+ int i, __count, err; > >> > >> *eof = 1; > >>+ __count = count; > >> count = 0; > >> > >> read_lock_bh(&osf_lock); > >>@@ -613,10 +616,13 @@ > >> > >> log("%s [%s]", f->genre, f->details); > >> > >>- count += sprintf(buf+count, "%s - %s[%s] : %s", > >>+ err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", > >> f->genre, f->version, > >> f->subtype, f->details); > >>- > >>+ if (err == 0) > >>+ break; > >>+ else > >>+ count += err; > >> if (f->opt_num) > >> { > >> loga(" OPT: "); > >>@@ -630,7 +636,11 @@ > >> } > >> } > >> loga("\n"); > >>- count += sprintf(buf+count, "\n"); > >>+ err = snprintf(buf+count, __count-count, "\n"); > >>+ if (err == 0) > >>+ break; > >>+ else > >>+ count += err; > >> } > >> read_unlock_bh(&osf_lock); > >> > >> > >> > >>------------------------------------------------------------------------ > >> > >>--- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 > >>+++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 > >>@@ -182,7 +185,6 @@ > >> optsize = tcp->doff*4 - sizeof(struct tcphdr); > >> } > >> > >>- > >> /* Actually we can create hash/table of all genres and search > >> * only in appropriate part, but here is initial variant, > >> * so will use slow path. > >>@@ -601,9 +603,10 @@ > >> { > >> struct list_head *ent; > >> struct osf_finger *f = NULL; > >>- int i; > >>+ int i, __count, err; > >> > >> *eof = 1; > >>+ __count = count; > >> count = 0; > >> > >> read_lock_bh(&osf_lock); > >>@@ -613,10 +616,13 @@ > >> > >> log("%s [%s]", f->genre, f->details); > >> > >>- count += sprintf(buf+count, "%s - %s[%s] : %s", > >>+ err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", > >> f->genre, f->version, > >> f->subtype, f->details); > >>- > >>+ if (err == 0) > >>+ break; > >>+ else > >>+ count += err; > >> if (f->opt_num) > >> { > >> loga(" OPT: "); > >>@@ -630,7 +636,11 @@ > >> } > >> } > >> loga("\n"); > >>- count += sprintf(buf+count, "\n"); > >>+ err = snprintf(buf+count, __count-count, "\n"); > >>+ if (err == 0) > >>+ break; > >>+ else > >>+ count += err; > >> } > >> read_unlock_bh(&osf_lock); > >> > >> > >> -- Evgeniy Polyakov ( s0mbre ) Crash is better than data corruption. -- Art Grabowski [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-23 10:30 ` Evgeniy Polyakov @ 2004-08-23 10:38 ` Henrik Nordstrom 2004-08-23 10:39 ` Evgeniy Polyakov 1 sibling, 0 replies; 11+ messages in thread From: Henrik Nordstrom @ 2004-08-23 10:38 UTC (permalink / raw) To: Evgeniy Polyakov; +Cc: Patrick McHardy, Harald Welte, netfilter-devel On Mon, 23 Aug 2004, Evgeniy Polyakov wrote: > If it truncates than we have [avoided] overflow and definetely will not > write anything after it(except zero-lengh snprintf) since > __count-count == 0 there. Not quite.. __count-count <= 0 there.. char buf[4]; n = snprintf(buf, sizeof(buf), "xxxxxxxx"); returns 8, indicating that buf needs to be at least 9 octets large for this call to succeed. Regards Henrik ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-23 10:30 ` Evgeniy Polyakov 2004-08-23 10:38 ` Henrik Nordstrom @ 2004-08-23 10:39 ` Evgeniy Polyakov 2004-08-23 11:35 ` Evgeniy Polyakov 1 sibling, 1 reply; 11+ messages in thread From: Evgeniy Polyakov @ 2004-08-23 10:39 UTC (permalink / raw) To: Patrick McHardy; +Cc: Henrik Nordstrom, Harald Welte, netfilter-devel [-- Attachment #1.1: Type: text/plain, Size: 1075 bytes --] On Mon, 2004-08-23 at 14:30, Evgeniy Polyakov wrote: > On Mon, 2004-08-23 at 13:55, Patrick McHardy wrote: > > Evgeniy Polyakov wrote: > > > > >It simply checks if return value from snprintf is 0 and breaks, > > >otherwise it proceeds. > > > > > Still broken. snprintf returns a value > n if it truncated to n bytes. > > See my last mail again. BTW, did the overflow actually cause problems ? > > proc has an extra k of space just for overflows .. > > If it truncates than we have [avoided] overflow and definetely will not > write anything after it(except zero-lengh snprintf) since > __count-count == 0 there. Actually <= 0 which is not good but avoids overflows. I can trigger overflow without patch(actually it was hard lockup without any messages). > Do you mean following: > list_for_each() > { > snprintf(); > if (count > __count) > break; > } Attached with check __count >= count + err; > > > > > Regards > > Patrick -- Evgeniy Polyakov ( s0mbre ) Crash is better than data corruption. -- Art Grabowski [-- Attachment #1.2: ipt_osf.diff.1 --] [-- Type: text/plain, Size: 1322 bytes --] --- netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 +++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 @@ -182,7 +185,6 @@ optsize = tcp->doff*4 - sizeof(struct tcphdr); } - /* Actually we can create hash/table of all genres and search * only in appropriate part, but here is initial variant, * so will use slow path. @@ -601,9 +603,10 @@ { struct list_head *ent; struct osf_finger *f = NULL; - int i; + int i, __count, err; *eof = 1; + __count = count; count = 0; read_lock_bh(&osf_lock); @@ -613,10 +616,13 @@ log("%s [%s]", f->genre, f->details); - count += sprintf(buf+count, "%s - %s[%s] : %s", + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", f->genre, f->version, f->subtype, f->details); - + if (err == 0 || __count >= count + err) + break; + else + count += err; if (f->opt_num) { loga(" OPT: "); @@ -630,7 +636,11 @@ } } loga("\n"); - count += sprintf(buf+count, "\n"); + err = snprintf(buf+count, __count-count, "\n"); + if (err == 0 || __count >= count + err) + break; + else + count += err; } read_unlock_bh(&osf_lock); [-- Attachment #1.3: ipt_osf.diff.1.24 --] [-- Type: text/plain, Size: 1322 bytes --] --- netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 +++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.4/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 @@ -182,7 +185,6 @@ optsize = tcp->doff*4 - sizeof(struct tcphdr); } - /* Actually we can create hash/table of all genres and search * only in appropriate part, but here is initial variant, * so will use slow path. @@ -601,9 +603,10 @@ { struct list_head *ent; struct osf_finger *f = NULL; - int i; + int i, __count, err; *eof = 1; + __count = count; count = 0; read_lock_bh(&osf_lock); @@ -613,10 +616,13 @@ log("%s [%s]", f->genre, f->details); - count += sprintf(buf+count, "%s - %s[%s] : %s", + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", f->genre, f->version, f->subtype, f->details); - + if (err == 0 || __count >= count + err) + break; + else + count += err; if (f->opt_num) { loga(" OPT: "); @@ -630,7 +636,11 @@ } } loga("\n"); - count += sprintf(buf+count, "\n"); + err = snprintf(buf+count, __count-count, "\n"); + if (err == 0 || __count >= count + err) + break; + else + count += err; } read_unlock_bh(&osf_lock); [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-23 10:39 ` Evgeniy Polyakov @ 2004-08-23 11:35 ` Evgeniy Polyakov 2004-08-23 18:33 ` Patrick McHardy 0 siblings, 1 reply; 11+ messages in thread From: Evgeniy Polyakov @ 2004-08-23 11:35 UTC (permalink / raw) To: Patrick McHardy; +Cc: Henrik Nordstrom, Harald Welte, netfilter-devel [-- Attachment #1.1: Type: text/plain, Size: 1277 bytes --] On Mon, 2004-08-23 at 14:39, Evgeniy Polyakov wrote: > On Mon, 2004-08-23 at 14:30, Evgeniy Polyakov wrote: > > On Mon, 2004-08-23 at 13:55, Patrick McHardy wrote: > > > Evgeniy Polyakov wrote: > > > > > > >It simply checks if return value from snprintf is 0 and breaks, > > > >otherwise it proceeds. > > > > > > > Still broken. snprintf returns a value > n if it truncated to n bytes. > > > See my last mail again. BTW, did the overflow actually cause problems ? > > > proc has an extra k of space just for overflows .. > > > > If it truncates than we have [avoided] overflow and definetely will not > > write anything after it(except zero-lengh snprintf) since > > __count-count == 0 there. > > Actually <= 0 which is not good but avoids overflows. > I can trigger overflow without patch(actually it was hard lockup without > any messages). > > > Do you mean following: > > list_for_each() > > { > > snprintf(); > > if (count > __count) > > break; > > } > > Attached with check > __count >= count + err; I'm not smoking bad crack, but it needs to be __count <= count + err; Attached. > > > > > > > > Regards > > > Patrick -- Evgeniy Polyakov ( s0mbre ) Crash is better than data corruption. -- Art Grabowski [-- Attachment #1.2: ipt_osf.diff --] [-- Type: text/x-patch, Size: 410 bytes --] --- netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-07-18 00:10:43.000000000 +0400 +++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-20 21:55:22.000000000 +0400 @@ -411,8 +413,11 @@ } } - read_unlock(&osf_lock); + if (fcount) + fmatch = FMATCH_OK; + read_unlock(&osf_lock); + return (fmatch == FMATCH_OK)?1:0; } [-- Attachment #1.3: ipt_osf.diff.1 --] [-- Type: text/plain, Size: 1322 bytes --] --- netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-22 00:54:44.000000000 +0400 +++ netfilter_cvs/patch-o-matic-ng/osf/linux-2.6/net/ipv4/netfilter/ipt_osf.c 2004-08-20 22:36:24.000000000 +0400 @@ -182,7 +185,6 @@ optsize = tcp->doff*4 - sizeof(struct tcphdr); } - /* Actually we can create hash/table of all genres and search * only in appropriate part, but here is initial variant, * so will use slow path. @@ -601,9 +603,10 @@ { struct list_head *ent; struct osf_finger *f = NULL; - int i; + int i, __count, err; *eof = 1; + __count = count; count = 0; read_lock_bh(&osf_lock); @@ -613,10 +616,13 @@ log("%s [%s]", f->genre, f->details); - count += sprintf(buf+count, "%s - %s[%s] : %s", + err = snprintf(buf+count, __count-count, "%s - %s[%s] : %s", f->genre, f->version, f->subtype, f->details); - + if (err == 0 || __count <= count + err) + break; + else + count += err; if (f->opt_num) { loga(" OPT: "); @@ -630,7 +636,11 @@ } } loga("\n"); - count += sprintf(buf+count, "\n"); + err = snprintf(buf+count, __count-count, "\n"); + if (err == 0 || __count <= count + err) + break; + else + count += err; } read_unlock_bh(&osf_lock); [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 189 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [2/2] osf: fixed /proc reading bug 2004-08-23 11:35 ` Evgeniy Polyakov @ 2004-08-23 18:33 ` Patrick McHardy 0 siblings, 0 replies; 11+ messages in thread From: Patrick McHardy @ 2004-08-23 18:33 UTC (permalink / raw) To: johnpol; +Cc: netfilter-devel Evgeniy Polyakov wrote: >I'm not smoking bad crack, but it needs to be __count <= count + err; >Attached. > > This one looks fine, applied. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2004-08-23 18:33 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2004-08-21 21:03 [2/2] osf: fixed /proc reading bug Evgeniy Polyakov 2004-08-21 22:30 ` Patrick McHardy 2004-08-21 23:48 ` Henrik Nordstrom 2004-08-22 0:15 ` Patrick McHardy 2004-08-23 8:57 ` Evgeniy Polyakov 2004-08-23 9:55 ` Patrick McHardy 2004-08-23 10:30 ` Evgeniy Polyakov 2004-08-23 10:38 ` Henrik Nordstrom 2004-08-23 10:39 ` Evgeniy Polyakov 2004-08-23 11:35 ` Evgeniy Polyakov 2004-08-23 18:33 ` Patrick McHardy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.