Date: Tue, 4 Sep 2001 09:50:06 -0700 (PDT) From: Jin Guojun <j_guojun@lbl.gov> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/16644: Bad comparsion expression in bpf_filter.c Message-ID: <200109041650.f84Go6X90120@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: Jin Guojun <j_guojun@lbl.gov>
To: Bruce Evans <bde@zeta.org.au>
Cc: mike@FreeBSD.ORG, freebsd-gnats-submit@FreeBSD.ORG
Subject: Re: kern/16644: Bad comparsion expression in bpf_filter.c
Date: Tue, 04 Sep 2001 09:47:03 -0700
Bruce Evans wrote:
> 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
The entire thing is messed up because the k is revised in the lastest BPF
release.
k is signed in the lastest release, and all above explanation is beased on signed
K.
It fixed the undefined overflow problem. Signed K does not work correctly,
that is why you had above overflow issue. Overflow is wrong (not intended).
Below is I explained to someone who asked about is:
-------- original message -----------
Jonathan Lemon wrote:
> Hello -
>
> I'm looking at bpf in FreeBSD, and it appears that our current
> definition for the bpf_insn structure is:
>
> struct bpf_insn {
> u_short code;
> u_char jt;
> u_char jf;
> bpf_u_int32 k;
> };
>
> Note the last line has ``k'' being unsigned. In the LBL releases,
> it seems that there was a bit of confusion on the definition of
> this field:
>
> original berkeley release: long
> LBL rev 1.34: bpf_u_int32
> LBL rev 1.36: bpf_int32
>
> Not having the RCS logs for the file, its not entirely clear
> for the differing definitions here. Is it intended that 'k'
> may be negative in the bpf_insn structure?
> --
> Jonathan
Yes, k may have negtive value. I cannot recall when FreeBSD changed it,
but I remember I read the email about it regarding some expression.
They claimed make k non-negtive can fix their problem, I do not know what.
Later sometime, I checked their expression which is bogus, so I send email
to the bugs@freebsd.org, and explained that expression has nothing to do
with int k or uint k. I asked them to change back because uint k breaks the
kernel BPF functioning. No one was responsed.
So, I put Kernel BPF patch on my web site in case some encounts problems.
-Jin
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?200109041650.f84Go6X90120>