bpf_validate() needs to do more checks
Sepherosa Ziehau
sepherosa at gmail.com
Wed Jan 2 04:33:05 PST 2008
On Jan 2, 2008 11:05 AM, Guy Harris <guy at alum.mit.edu> wrote:
> OpenBSD's bpf_validate() in sys/net/bpf_filter.c does some additional
> checks to catch:
>
> BPF programs with no instructions or with more than BPF_MAXINSNS
> instructions;
>
> BPF_STX and BPF_LDX|BPF_MEM instructions that have out-of-range offsets
> (which could be made to fetch or store into arbitrary memory locations);
>
> BPF_DIV instructions with a constant 0 divisor (that's a check also done
> at run time).
>
> Here's a patch to make the DragonFly BSD bpf_validate() match it.
Committed. Thanks!
As I said in the previous mail: next time, unified diff please :)
Best Regards,
sephe
>
> *** bpf_filter.c Tue Jan 1 18:57:43 2008
> --- bpf_filter.c.VALIDATE Tue Jan 1 19:00:15 2008
> ***************
> *** 498,505 ****
> #ifdef _KERNEL
> /*
> * Return true if the 'fcode' is a valid filter program.
> ! * The constraints are that jump and memory accesses are within valid
> ! * ranges, 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.
> --- 498,508 ----
> #ifdef _KERNEL
> /*
> * Return true if the 'fcode' is a valid filter program.
> ! * The constraints are that each jump be forward and to a valid
> ! * 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.
> ***************
> *** 507,544 ****
> int
> bpf_validate(const struct bpf_insn *f, int len)
> {
> ! int i;
> const struct bpf_insn *p;
>
> for (i = 0; i < len; ++i) {
> /*
> ! * Check that that jumps are within the code block.
> */
> ! p = &f[i];
> ! if (BPF_CLASS(p->code) == BPF_JMP) {
> ! int from = i + 1;
> !
> ! if (BPF_OP(p->code) == BPF_JA) {
> ! if (from >= len || p->k >= len - from)
> return 0;
> }
> ! else if (from >= len || p->jt >= len - from ||
> ! p->jf >= len - from)
> 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)
> return 0;
> }
> return BPF_CLASS(f[len - 1].code) == BPF_RET;
> }
> --- 510,620 ----
> int
> bpf_validate(const struct bpf_insn *f, int len)
> {
> ! 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 memory operations use valid addresses.
> */
> ! 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;
> }
> ! break;
> ! case BPF_ST:
> ! case BPF_STX:
> ! if (p->k >= BPF_MEMWORDS)
> return 0;
> ! break;
> ! case BPF_ALU:
> ! switch (BPF_OP(p->code)) {
> ! case BPF_ADD:
> ! case BPF_SUB:
> ! case BPF_MUL:
> ! 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;
> ! break;
> ! 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;
> }
>
--
Live Free or Die
More information about the Bugs
mailing list