* [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.