Skip to content

Commit 588a25e

Browse files
Alexei Starovoitovborkmann
authored andcommitted
bpf: Fix extable address check.
The verifier checks that PTR_TO_BTF_ID pointer is either valid or NULL, but it cannot distinguish IS_ERR pointer from valid one. When offset is added to IS_ERR pointer it may become small positive value which is a user address that is not handled by extable logic and has to be checked for at the runtime. Tighten BPF_PROBE_MEM pointer check code to prevent this case. Fixes: 4c5de12 ("bpf: Emit explicit NULL pointer checks for PROBE_LDX instructions.") Reported-by: Lorenzo Fontana <[email protected]> Signed-off-by: Alexei Starovoitov <[email protected]> Acked-by: Daniel Borkmann <[email protected]> Signed-off-by: Daniel Borkmann <[email protected]>
1 parent 433956e commit 588a25e

File tree

1 file changed

+42
-7
lines changed

1 file changed

+42
-7
lines changed

arch/x86/net/bpf_jit_comp.c

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1252,19 +1252,54 @@ st: if (is_imm8(insn->off))
12521252
case BPF_LDX | BPF_MEM | BPF_DW:
12531253
case BPF_LDX | BPF_PROBE_MEM | BPF_DW:
12541254
if (BPF_MODE(insn->code) == BPF_PROBE_MEM) {
1255-
/* test src_reg, src_reg */
1256-
maybe_emit_mod(&prog, src_reg, src_reg, true); /* always 1 byte */
1257-
EMIT2(0x85, add_2reg(0xC0, src_reg, src_reg));
1258-
/* jne start_of_ldx */
1259-
EMIT2(X86_JNE, 0);
1255+
/* Though the verifier prevents negative insn->off in BPF_PROBE_MEM
1256+
* add abs(insn->off) to the limit to make sure that negative
1257+
* offset won't be an issue.
1258+
* insn->off is s16, so it won't affect valid pointers.
1259+
*/
1260+
u64 limit = TASK_SIZE_MAX + PAGE_SIZE + abs(insn->off);
1261+
u8 *end_of_jmp1, *end_of_jmp2;
1262+
1263+
/* Conservatively check that src_reg + insn->off is a kernel address:
1264+
* 1. src_reg + insn->off >= limit
1265+
* 2. src_reg + insn->off doesn't become small positive.
1266+
* Cannot do src_reg + insn->off >= limit in one branch,
1267+
* since it needs two spare registers, but JIT has only one.
1268+
*/
1269+
1270+
/* movabsq r11, limit */
1271+
EMIT2(add_1mod(0x48, AUX_REG), add_1reg(0xB8, AUX_REG));
1272+
EMIT((u32)limit, 4);
1273+
EMIT(limit >> 32, 4);
1274+
/* cmp src_reg, r11 */
1275+
maybe_emit_mod(&prog, src_reg, AUX_REG, true);
1276+
EMIT2(0x39, add_2reg(0xC0, src_reg, AUX_REG));
1277+
/* if unsigned '<' goto end_of_jmp2 */
1278+
EMIT2(X86_JB, 0);
1279+
end_of_jmp1 = prog;
1280+
1281+
/* mov r11, src_reg */
1282+
emit_mov_reg(&prog, true, AUX_REG, src_reg);
1283+
/* add r11, insn->off */
1284+
maybe_emit_1mod(&prog, AUX_REG, true);
1285+
EMIT2_off32(0x81, add_1reg(0xC0, AUX_REG), insn->off);
1286+
/* jmp if not carry to start_of_ldx
1287+
* Otherwise ERR_PTR(-EINVAL) + 128 will be the user addr
1288+
* that has to be rejected.
1289+
*/
1290+
EMIT2(0x73 /* JNC */, 0);
1291+
end_of_jmp2 = prog;
1292+
12601293
/* xor dst_reg, dst_reg */
12611294
emit_mov_imm32(&prog, false, dst_reg, 0);
12621295
/* jmp byte_after_ldx */
12631296
EMIT2(0xEB, 0);
12641297

1265-
/* populate jmp_offset for JNE above */
1266-
temp[4] = prog - temp - 5 /* sizeof(test + jne) */;
1298+
/* populate jmp_offset for JB above to jump to xor dst_reg */
1299+
end_of_jmp1[-1] = end_of_jmp2 - end_of_jmp1;
1300+
/* populate jmp_offset for JNC above to jump to start_of_ldx */
12671301
start_of_ldx = prog;
1302+
end_of_jmp2[-1] = start_of_ldx - end_of_jmp2;
12681303
}
12691304
emit_ldx(&prog, BPF_SIZE(insn->code), dst_reg, src_reg, insn->off);
12701305
if (BPF_MODE(insn->code) == BPF_PROBE_MEM) {

0 commit comments

Comments
 (0)