Date: Sat, 1 Sep 2001 02:10:01 -0700 (PDT) From: Bruce Evans <bde@zeta.org.au> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/16644: Bad comparsion expression in bpf_filter.c Message-ID: <200109010910.f819A1v53924@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/16644; it has been noted by GNATS.
From: Bruce Evans <bde@zeta.org.au>
To: "Jin Guojun[ITG]" <j_guojun@lbl.gov>
Cc: <mike@FreeBSD.ORG>, <freebsd-gnats-submit@FreeBSD.ORG>
Subject: Re: kern/16644: Bad comparsion expression in bpf_filter.c
Date: Sat, 1 Sep 2001 19:01:12 +1000 (EST)
On Fri, 31 Aug 2001, Jin Guojun[ITG] wrote:
> It is still there. I have replied this to the discussion and
> got no response. For example, in line 220, ">" line is equal to
> if (k > buflen || k + sizeof(int32_t) > buflen) {
> or
> if (k > buflen || k > buflen - sizeof(int32_t)) {
>
> if K > BUFLEN then K must > BUFLEN - 4
This doesn't follow. Example: K = 4U, BUFLEN = 2U, BUFLEN - 4 = UINT_MAX - 2.
BUFLEN - 4 overflows. I'm not sure if this overflow can give undefined
behaviour instead of UINT_MAX - 2.
> so we only want to judge if (k > buflen - sizeof(int32_t)) {
> which is the "<" of line 220 -- if (k + sizeof(int32_t) > buflen) {
>
> Right? rests are ditto. The original design is correct.
No. Examples:
(1) k = UINT_MAX - 2, buflen = 8U. Then k + sizeof(int32_t) = 2U, which
is <= buflen. But k is much larger than buflen - sizeof(int32_t).
Here arithmetic overflow occurs in the buffer overflow check and
the buffer overflow check gives the wrong result. The overflow
checks were rewritten in FreeBSD to avoid arithmetic overflow.
(2) k = -2 (signed int, as in the original design), buflen = 8U. Then
k + sizeof(int32_t) = 2U (no change). This is the same overflow as
in (1). k first gets promoted to UINT_MAX - 2, then the addition
overflows.
> The real problem is at line 550. K is outside 0-BPF_MEMWORDS, not just >.
> ...
> 547c550
> < (p->k >= BPF_MEMWORDS || p->k < 0))
> ---
> > p->k >= BPF_MEMWORDS)
This patch is reversed (it gives the FreeBSD change). Here FreeBSD
simplified the check instead of complicating it. p->k < 0 can't happen
because p->k is unsigned. The compiler should optimize away the
complication and generate identical code, but it migh warn about a
bogus comparison of an unsigned value with 0.
Bruce
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-bugs" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200109010910.f819A1v53924>