Kris Katterjohn wrote:
--- x/net/core/filter.c 2005-12-28 16:51:35.000000000 -0600
+++ y/net/core/filter.c 2005-12-28 16:53:32.000000000 -0600
@@ -250,7 +250,7 @@ load_b:
mem[fentry->k] = X;
continue;
default:
- /* Invalid instruction counts as RET */
+ /* Should never be reached */
This stuff has had a number of bad bugs before, if it can't be reached,
please call BUG or something similar instead of silently ignoring the
error.
return 0;
}
@@ -300,38 +300,87 @@ int sk_chk_filter(struct sock_filter *fi
for (pc = 0; pc < flen; pc++) {
/* all jumps are forward as they are not signed */
ftest = &filter[pc];
- if (BPF_CLASS(ftest->code) == BPF_JMP) {
- /* but they mustn't jump off the end */
- if (BPF_OP(ftest->code) == BPF_JA) {
- /*
- * Note, the large ftest->k might cause loops.
- * Compare this with conditional jumps below,
- * where offsets are limited. --ANK (981016)
- */
- if (ftest->k >= (unsigned)(flen-pc-1))
- return -EINVAL;
- } else {
- /* for conditionals both must be safe */
- if (pc + ftest->jt +1 >= flen ||
- pc + ftest->jf +1 >= flen)
- return -EINVAL;
- }
- }
- /* check for division by zero -Kris Katterjohn 2005-10-30 */
- if (ftest->code == (BPF_ALU|BPF_DIV|BPF_K) && ftest->k == 0)
- return -EINVAL;
+ /* Only allow valid instructions -Kris Katterjohn 2005-12-28 */
+ switch (ftest->code) {
+ case BPF_ALU|BPF_ADD|BPF_K:
+ case BPF_ALU|BPF_ADD|BPF_X:
+ case BPF_ALU|BPF_SUB|BPF_K:
+ case BPF_ALU|BPF_SUB|BPF_X:
+ case BPF_ALU|BPF_MUL|BPF_K:
+ case BPF_ALU|BPF_MUL|BPF_X:
+ case BPF_ALU|BPF_DIV|BPF_X:
+ case BPF_ALU|BPF_AND|BPF_K:
+ case BPF_ALU|BPF_AND|BPF_X:
+ case BPF_ALU|BPF_OR|BPF_K:
+ case BPF_ALU|BPF_OR|BPF_X:
+ case BPF_ALU|BPF_LSH|BPF_K:
+ case BPF_ALU|BPF_LSH|BPF_X:
+ case BPF_ALU|BPF_RSH|BPF_K:
+ case BPF_ALU|BPF_RSH|BPF_X:
+ case BPF_ALU|BPF_NEG:
+ case BPF_LD|BPF_W|BPF_ABS:
+ case BPF_LD|BPF_H|BPF_ABS:
+ case BPF_LD|BPF_B|BPF_ABS:
+ case BPF_LD|BPF_W|BPF_LEN:
+ case BPF_LD|BPF_W|BPF_IND:
+ case BPF_LD|BPF_H|BPF_IND:
+ case BPF_LD|BPF_B|BPF_IND:
+ case BPF_LD|BPF_IMM:
+ case BPF_LDX|BPF_W|BPF_LEN:
+ case BPF_LDX|BPF_B|BPF_MSH:
+ case BPF_LDX|BPF_IMM:
+ case BPF_MISC|BPF_TAX:
+ case BPF_MISC|BPF_TXA:
+ case BPF_RET|BPF_K:
+ case BPF_RET|BPF_A:
+ break;
I think this could be done more readable using BPF_CLASS().
+
+ /* Some instructions need special checks */
+
+ case BPF_ALU|BPF_DIV|BPF_K:
+ /* check for division by zero
+ * -Kris Katterjohn 2005-10-30
Please don't annotate every single comment with your name and date,
especially not totally useless ones such as this. If you want some
record of your changes inside the file, place it somewhere at the
top, where it doesn't clutter up the code.
+ */
+ if (ftest->k == 0)
+ return -EINVAL;
Why do you keep the runtime check then?
+ break;
+
+ case BPF_LD|BPF_MEM:
+ case BPF_LDX|BPF_MEM:
+ case BPF_ST:
+ case BPF_STX:
+ /* check for invalid memory addresses */
+ if (ftest->k >= BPF_MEMWORDS)
+ return -EINVAL;
+ break;
- /* check that memory operations use valid addresses. */
- if (ftest->k >= BPF_MEMWORDS) {
- /* but it might not be a memory operation... */
- switch (ftest->code) {
- case BPF_ST:
- case BPF_STX:
- case BPF_LD|BPF_MEM:
- case BPF_LDX|BPF_MEM:
+ case BPF_JMP|BPF_JA:
+ /*
+ * Note, the large ftest->k might cause loops.
+ * Compare this with conditional jumps below,
+ * where offsets are limited. --ANK (981016)
+ */
+ if (ftest->k >= (unsigned)(flen-pc-1))
return -EINVAL;
- }
+ break;
+
+ case BPF_JMP|BPF_JEQ|BPF_K:
+ case BPF_JMP|BPF_JEQ|BPF_X:
+ case BPF_JMP|BPF_JGE|BPF_K:
+ case BPF_JMP|BPF_JGE|BPF_X:
+ case BPF_JMP|BPF_JGT|BPF_K:
+ case BPF_JMP|BPF_JGT|BPF_X:
+ case BPF_JMP|BPF_JSET|BPF_K:
+ case BPF_JMP|BPF_JSET|BPF_X:
+ /* for conditionals both must be safe */
+ if (pc + ftest->jt + 1 >= flen ||
+ pc + ftest->jf + 1 >= flen)
+ return -EINVAL;
+ break;
+
+ default:
+ return -EINVAL;
}
}
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
