From owner-freebsd-bugs Tue Sep 4 9:50:16 2001 Delivered-To: freebsd-bugs@hub.freebsd.org Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by hub.freebsd.org (Postfix) with ESMTP id CA68B37B40B for ; Tue, 4 Sep 2001 09:50:06 -0700 (PDT) Received: (from gnats@localhost) by freefall.freebsd.org (8.11.4/8.11.4) id f84Go6X90120; Tue, 4 Sep 2001 09:50:06 -0700 (PDT) (envelope-from gnats) Date: Tue, 4 Sep 2001 09:50:06 -0700 (PDT) Message-Id: <200109041650.f84Go6X90120@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org Cc: From: Jin Guojun Subject: Re: kern/16644: Bad comparsion expression in bpf_filter.c Reply-To: Jin Guojun Sender: owner-freebsd-bugs@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.org The following reply was made to PR kern/16644; it has been noted by GNATS. From: Jin Guojun To: Bruce Evans 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