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>