Date: Wed, 30 Nov 2005 11:30:07 GMT From: Guy Harris <guy@alum.mit.edu> To: freebsd-bugs@FreeBSD.org Subject: Re: kern/89752: bpf_validate() needs to do more checks Message-ID: <200511301130.jAUBU7Qd040178@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR kern/89752; it has been noted by GNATS.
From: Guy Harris <guy@alum.mit.edu>
To: bug-followup@FreeBSD.org, guy@alum.mit.edu
Cc:
Subject: Re: kern/89752: bpf_validate() needs to do more checks
Date: Wed, 30 Nov 2005 03:25:22 -0800
This is a multi-part message in MIME format.
--------------030206000405030002050805
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
I've attached a unified diff for the patch, just in case the patch got
tabs mangled to spaces (I submitted the bug via the Web).
--------------030206000405030002050805
Content-Type: text/plain; x-mac-type="0"; x-mac-creator="0";
name="patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="patch"
Index: bpf_filter.c
===================================================================
RCS file: /cvs/root/System/xnu/bsd/net/bpf_filter.c,v
retrieving revision 1.8
diff -u -r1.8 bpf_filter.c
--- bpf_filter.c 2004/04/01 23:28:48 1.8
+++ bpf_filter.c 2005/11/30 11:23:17
@@ -522,9 +522,10 @@
/*
* Return true if the 'fcode' is a valid filter program.
* The constraints are that each jump be forward and to a valid
- * code. The code must terminate with either an accept or reject.
- * 'valid' is an array for use by the routine (it must be at least
- * 'len' bytes long).
+ * code, that memory accesses are within valid ranges (to the
+ * extent that this can be checked statically; loads of packet
+ * data have to be, and are, also checked at run time), and that
+ * the code terminates with either an accept or reject.
*
* The kernel needs to be able to verify an application's filter code.
* Otherwise, a bogus program could easily crash the system.
@@ -532,39 +533,109 @@
int
bpf_validate(const struct bpf_insn *f, int len)
{
- register int i;
+ u_int i, from;
const struct bpf_insn *p;
+ if (len < 1 || len > BPF_MAXINSNS)
+ return 0;
+
for (i = 0; i < len; ++i) {
+ p = &f[i];
+ switch (BPF_CLASS(p->code)) {
/*
- * Check that that jumps are forward, and within
- * the code block.
+ * Check that memory operations use valid addresses.
*/
- p = &f[i];
- if (BPF_CLASS(p->code) == BPF_JMP) {
- register int from = i + 1;
-
- if (BPF_OP(p->code) == BPF_JA) {
- if (from >= len || p->k >= (bpf_u_int32)(len - from))
+ case BPF_LD:
+ case BPF_LDX:
+ switch (BPF_MODE(p->code)) {
+ case BPF_IMM:
+ break;
+ case BPF_ABS:
+ case BPF_IND:
+ case BPF_MSH:
+ /*
+ * More strict check with actual packet length
+ * is done runtime.
+ */
+ if (p->k >= bpf_maxbufsize)
+ return 0;
+ break;
+ case BPF_MEM:
+ if (p->k >= BPF_MEMWORDS)
return 0;
+ break;
+ case BPF_LEN:
+ break;
+ default:
+ return 0;
}
- else if (from >= len || p->jt >= len - from ||
- p->jf >= len - from)
+ break;
+ case BPF_ST:
+ case BPF_STX:
+ if (p->k >= BPF_MEMWORDS)
return 0;
- }
- /*
- * Check that memory operations use valid addresses.
- */
- if ((BPF_CLASS(p->code) == BPF_ST ||
- (BPF_CLASS(p->code) == BPF_LD &&
- (p->code & 0xe0) == BPF_MEM)) &&
- p->k >= BPF_MEMWORDS)
- return 0;
- /*
- * Check for constant division by 0.
- */
- if (p->code == (BPF_ALU|BPF_DIV|BPF_K) && p->k == 0)
+ break;
+ case BPF_ALU:
+ switch (BPF_OP(p->code)) {
+ case BPF_ADD:
+ case BPF_SUB:
+ case BPF_OR:
+ case BPF_AND:
+ case BPF_LSH:
+ case BPF_RSH:
+ case BPF_NEG:
+ break;
+ case BPF_DIV:
+ /*
+ * Check for constant division by 0.
+ */
+ if (BPF_RVAL(p->code) == BPF_K && p->k == 0)
+ return 0;
+ default:
+ return 0;
+ }
+ break;
+ case BPF_JMP:
+ /*
+ * Check that jumps are within the code block,
+ * and that unconditional branches don't go
+ * backwards as a result of an overflow.
+ * Unconditional branches have a 32-bit offset,
+ * so they could overflow; we check to make
+ * sure they don't. Conditional branches have
+ * an 8-bit offset, and the from address is <=
+ * BPF_MAXINSNS, and we assume that BPF_MAXINSNS
+ * is sufficiently small that adding 255 to it
+ * won't overflow.
+ *
+ * We know that len is <= BPF_MAXINSNS, and we
+ * assume that BPF_MAXINSNS is < the maximum size
+ * of a u_int, so that i + 1 doesn't overflow.
+ */
+ from = i + 1;
+ switch (BPF_OP(p->code)) {
+ case BPF_JA:
+ if (from + p->k < from || from + p->k >= len)
+ return 0;
+ break;
+ case BPF_JEQ:
+ case BPF_JGT:
+ case BPF_JGE:
+ case BPF_JSET:
+ if (from + p->jt >= len || from + p->jf >= len)
+ return 0;
+ break;
+ default:
+ return 0;
+ }
+ break;
+ case BPF_RET:
+ break;
+ case BPF_MISC:
+ break;
+ default:
return 0;
+ }
}
return BPF_CLASS(f[len - 1].code) == BPF_RET;
}
--------------030206000405030002050805--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200511301130.jAUBU7Qd040178>