Date: Tue, 4 Sep 2001 11:00:03 -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: <200109041800.f84I03r03259@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>, mike@FreeBSD.ORG,
freebsd-gnats-submit@FreeBSD.ORG
Cc:
Subject: Re: kern/16644: Bad comparsion expression in bpf_filter.c
Date: Tue, 04 Sep 2001 11:01:44 -0700
There was a typo in the previous message I sent.
< It fixed the undefined overflow problem. Signed K does not work correctly,
^^^^^
should be
> It fixed the undefined overflow problem. Unsigned K does not work correctly,
^^^^^^^
-Jin
Jin Guojun wrote:
> 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?200109041800.f84I03r03259>