History

bpf: fix range arithmetic for bpf map access

I made some invalid assumptions with BPF_AND and BPF_MOD that could result in invalid accesses to bpf map entries. Fix this up by doing a few things 1) Kill BPF_MOD support. This doesn't actually get used by the compiler in real life and just adds extra complexity. 2) Fix the logic for BPF_AND, don't allow AND of negative numbers and set the minimum value to 0 for positive AND's. 3) Don't do operations on the ranges if they are set to the limits, as they are by definition undefined, and allowing arithmetic operations on those values could make them appear valid when they really aren't. This fixes the testcase provided by Jann as well as a few other theoretical problems.

Josef Bacik committed 9 years ago

Commit f23cc641 parent b3cfaa32 files · +50 −25Browse files

Showing the changes to kernel (1 of 2 files). Show all

Unified · Split

kernel/bpf/verifier.c+47−23
@@ -216,8 +216,8 @@ static void print_verifier_state(struct bpf_verifier_state *state) · 215 unmodified lines
216216 reg->map_ptr->key_size,
217217 reg->map_ptr->value_size);
218218 if (reg->min_value != BPF_REGISTER_MIN_RANGE)
219− verbose(",min_value=%llu",
220− (unsigned long long)reg->min_value);
219+ verbose(",min_value=%lld",
220+ (long long)reg->min_value);
221221 if (reg->max_value != BPF_REGISTER_MAX_RANGE)
222222 verbose(",max_value=%llu",
223223 (unsigned long long)reg->max_value);
@@ -758,7 +758,7 @@ static int check_mem_access(struct bpf_verifier_env *env, u32 regno, int off, · 534 unmodified lines
758758 * index'es we need to make sure that whatever we use
759759 * will have a set floor within our range.
760760 */
761− if ((s64)reg->min_value < 0) {
761+ if (reg->min_value < 0) {
762762 verbose("R%d min value is negative, either use unsigned index or do a if (index >=0) check.\n",
763763 regno);
764764 return -EACCES;
@@ -1468,7 +1468,8 @@ static void check_reg_overflow(struct bpf_reg_state *reg) · 703 unmodified lines
14681468 {
14691469 if (reg->max_value > BPF_REGISTER_MAX_RANGE)
14701470 reg->max_value = BPF_REGISTER_MAX_RANGE;
1471− if ((s64)reg->min_value < BPF_REGISTER_MIN_RANGE)
1471+ if (reg->min_value < BPF_REGISTER_MIN_RANGE ||
1472+ reg->min_value > BPF_REGISTER_MAX_RANGE)
14721473 reg->min_value = BPF_REGISTER_MIN_RANGE;
14731474 }
14741475
@@ -1476,7 +1477,8 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env, · 1 unmodified line
14761477 struct bpf_insn *insn)
14771478 {
14781479 struct bpf_reg_state *regs = env->cur_state.regs, *dst_reg;
1479− u64 min_val = BPF_REGISTER_MIN_RANGE, max_val = BPF_REGISTER_MAX_RANGE;
1480+ s64 min_val = BPF_REGISTER_MIN_RANGE;
1481+ u64 max_val = BPF_REGISTER_MAX_RANGE;
14801482 bool min_set = false, max_set = false;
14811483 u8 opcode = BPF_OP(insn->code);
14821484
@@ -1512,22 +1514,43 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env, · 29 unmodified lines
15121514 return;
15131515 }
15141516
1517+ /* If one of our values was at the end of our ranges then we can't just
1518+ * do our normal operations to the register, we need to set the values
1519+ * to the min/max since they are undefined.
1520+ */
1521+ if (min_val == BPF_REGISTER_MIN_RANGE)
1522+ dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
1523+ if (max_val == BPF_REGISTER_MAX_RANGE)
1524+ dst_reg->max_value = BPF_REGISTER_MAX_RANGE;
1525+
15151526 switch (opcode) {
15161527 case BPF_ADD:
1517− dst_reg->min_value += min_val;
1518− dst_reg->max_value += max_val;
1528+ if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
1529+ dst_reg->min_value += min_val;
1530+ if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
1531+ dst_reg->max_value += max_val;
15191532 break;
15201533 case BPF_SUB:
1521− dst_reg->min_value -= min_val;
1522− dst_reg->max_value -= max_val;
1534+ if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
1535+ dst_reg->min_value -= min_val;
1536+ if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
1537+ dst_reg->max_value -= max_val;
15231538 break;
15241539 case BPF_MUL:
1525− dst_reg->min_value *= min_val;
1526− dst_reg->max_value *= max_val;
1540+ if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
1541+ dst_reg->min_value *= min_val;
1542+ if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
1543+ dst_reg->max_value *= max_val;
15271544 break;
15281545 case BPF_AND:
1529− /* & is special since it could end up with 0 bits set. */
1530− dst_reg->min_value &= min_val;
1546+ /* Disallow AND'ing of negative numbers, ain't nobody got time
1547+ * for that. Otherwise the minimum is 0 and the max is the max
1548+ * value we could AND against.
1549+ */
1550+ if (min_val < 0)
1551+ dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
1552+ else
1553+ dst_reg->min_value = 0;
15311554 dst_reg->max_value = max_val;
15321555 break;
15331556 case BPF_LSH:
@@ -1537,24 +1560,25 @@ static void adjust_reg_min_max_vals(struct bpf_verifier_env *env, · 3 unmodified lines
15371560 */
15381561 if (min_val > ilog2(BPF_REGISTER_MAX_RANGE))
15391562 dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
1540− else
1563+ else if (dst_reg->min_value != BPF_REGISTER_MIN_RANGE)
15411564 dst_reg->min_value <<= min_val;
15421565
15431566 if (max_val > ilog2(BPF_REGISTER_MAX_RANGE))
15441567 dst_reg->max_value = BPF_REGISTER_MAX_RANGE;
1545− else
1568+ else if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
15461569 dst_reg->max_value <<= max_val;
15471570 break;
15481571 case BPF_RSH:
1549− dst_reg->min_value >>= min_val;
1550− dst_reg->max_value >>= max_val;
1551− break;
1552− case BPF_MOD:
1553− /* % is special since it is an unsigned modulus, so the floor
1554− * will always be 0.
1572+ /* RSH by a negative number is undefined, and the BPF_RSH is an
1573+ * unsigned shift, so make the appropriate casts.
15551574 */
1556− dst_reg->min_value = 0;
1557− dst_reg->max_value = max_val - 1;
1575+ if (min_val < 0 || dst_reg->min_value < 0)
1576+ dst_reg->min_value = BPF_REGISTER_MIN_RANGE;
1577+ else
1578+ dst_reg->min_value =
1579+ (u64)(dst_reg->min_value) >> min_val;
1580+ if (dst_reg->max_value != BPF_REGISTER_MAX_RANGE)
1581+ dst_reg->max_value >>= max_val;
15581582 break;
15591583 default:
15601584 reset_reg_range_values(regs, insn->dst_reg);