From owner-freebsd-bugs Tue Sep 4 11: 0:12 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 7A1EE37B406 for ; Tue, 4 Sep 2001 11:00:03 -0700 (PDT) Received: (from gnats@localhost) by freefall.freebsd.org (8.11.4/8.11.4) id f84I03r03259; Tue, 4 Sep 2001 11:00:03 -0700 (PDT) (envelope-from gnats) Date: Tue, 4 Sep 2001 11:00:03 -0700 (PDT) Message-Id: <200109041800.f84I03r03259@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 , 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