Kuee reported a quirk in the jmp32's jeq/jne simulation, namely that the register value does not match expectations for the fall-through path. For example: Before fix: 0: R1=ctx(off=0,imm=0) R10=fp0 0: (b7) r2 = 0 ; R2_w=P0 1: (b7) r6 = 563 ; R6_w=P563 2: (87) r2 = -r2 ; R2_w=Pscalar() 3: (87) r2 = -r2 ; R2_w=Pscalar() 4: (4c) w2 |= w6 ; R2_w=Pscalar(umin=563,umax=4294967295,var_off=(0x233; 0xfffffdcc),s32_min=-2147483085) R6_w=P563 5: (56) if w2 != 0x8 goto pc+1 ; R2_w=P571 <--- [*] 6: (95) exit R0 !read_ok After fix: 0: R1=ctx(off=0,imm=0) R10=fp0 0: (b7) r2 = 0 ; R2_w=P0 1: (b7) r6 = 563 ; R6_w=P563 2: (87) r2 = -r2 ; R2_w=Pscalar() 3: (87) r2 = -r2 ; R2_w=Pscalar() 4: (4c) w2 |= w6 ; R2_w=Pscalar(umin=563,umax=4294967295,var_off=(0x233; 0xfffffdcc),s32_min=-2147483085) R6_w=P563 5: (56) if w2 != 0x8 goto pc+1 ; R2_w=P8 <--- [*] 6: (95) exit R0 !read_ok As can be seen on line 5 for the branch fall-through path in R2 [*] is that given condition w2 != 0x8 is false, verifier should conclude that r2 = 8 as upper 32 bit are known to be zero. However, verifier incorrectly concludes that r2 = 571 which is far off. The problem is it only marks false{true}_reg as known in the switch for JE/NE case, but at the end of the function, it uses {false,true}_{64,32}off to update {false,true}_reg->var_off and they still hold the prior value of {false,true}_reg->var_off before it got marked as known. The subsequent __reg_combine_32_into_64() then propagates this old var_off and derives new bounds. The information between min/max bounds on {false,true}_reg from setting the register to known const combined with the {false,true}_reg->var_off based on the old information then derives wrong register data. Fix it by detangling the BPF_JEQ/BPF_JNE cases and updating relevant {false,true}_{64,32}off tnums along with the register marking to known constant.
Showing the changes to kernel (1 of 1 file). Show all
Unified · Split
| @@ -9577,26 +9577,33 @@ static void reg_set_min_max(struct bpf_reg_state *true_reg, · 9576 unmodified lines | ||
| 9577 | 9577 | return; |
| 9578 | 9578 | |
| 9579 | 9579 | switch (opcode) { |
| 9580 | + /* JEQ/JNE comparison doesn't change the register equivalence. | |
| 9581 | + * | |
| 9582 | + * r1 = r2; | |
| 9583 | + * if (r1 == 42) goto label; | |
| 9584 | + * ... | |
| 9585 | + * label: // here both r1 and r2 are known to be 42. | |
| 9586 | + * | |
| 9587 | + * Hence when marking register as known preserve it's ID. | |
| 9588 | + */ | |
| 9580 | 9589 | case BPF_JEQ: |
| 9590 | + if (is_jmp32) { | |
| 9591 | + __mark_reg32_known(true_reg, val32); | |
| 9592 | + true_32off = tnum_subreg(true_reg->var_off); | |
| 9593 | + } else { | |
| 9594 | + ___mark_reg_known(true_reg, val); | |
| 9595 | + true_64off = true_reg->var_off; | |
| 9596 | + } | |
| 9597 | + break; | |
| 9581 | 9598 | case BPF_JNE: |
| 9582 | − { | |
| 9583 | − struct bpf_reg_state *reg = | |
| 9584 | − opcode == BPF_JEQ ? true_reg : false_reg; | |
| 9585 | − | |
| 9586 | − /* JEQ/JNE comparison doesn't change the register equivalence. | |
| 9587 | − * r1 = r2; | |
| 9588 | − * if (r1 == 42) goto label; | |
| 9589 | − * ... | |
| 9590 | − * label: // here both r1 and r2 are known to be 42. | |
| 9591 | − * | |
| 9592 | − * Hence when marking register as known preserve it's ID. | |
| 9593 | − */ | |
| 9594 | − if (is_jmp32) | |
| 9595 | − __mark_reg32_known(reg, val32); | |
| 9596 | − else | |
| 9597 | − ___mark_reg_known(reg, val); | |
| 9599 | + if (is_jmp32) { | |
| 9600 | + __mark_reg32_known(false_reg, val32); | |
| 9601 | + false_32off = tnum_subreg(false_reg->var_off); | |
| 9602 | + } else { | |
| 9603 | + ___mark_reg_known(false_reg, val); | |
| 9604 | + false_64off = false_reg->var_off; | |
| 9605 | + } | |
| 9598 | 9606 | break; |
| 9599 | − } | |
| 9600 | 9607 | case BPF_JSET: |
| 9601 | 9608 | if (is_jmp32) { |
| 9602 | 9609 | false_32off = tnum_and(false_32off, tnum_const(~val32)); |