Skip to content

Commit 0657a91

Browse files
jrfastabkernel-patches-bot
authored andcommitted
bpf: track subprog poke correctly
Subprograms are calling map_poke_track but on program release there is no hook to call map_poke_untrack. But on prog release the aux memory is freed even though we still have a reference to it in the element list of the map aux data. So when we run map_poke_run() we end up accessing free'd memory. This triggers with KASAN in prog_array_map_poke_run() shown here. [ 402.824686] ================================================================== [ 402.824689] BUG: KASAN: use-after-free in prog_array_map_poke_run+0xc2/0x34e [ 402.824698] Read of size 4 at addr ffff8881905a7940 by task hubble-fgs/4337 [ 402.824705] CPU: 1 PID: 4337 Comm: hubble-fgs Tainted: G I 5.12.0+ #399 [ 402.824715] Call Trace: [ 402.824719] dump_stack+0x93/0xc2 [ 402.824727] print_address_description.constprop.0+0x1a/0x140 [ 402.824736] ? prog_array_map_poke_run+0xc2/0x34e [ 402.824740] ? prog_array_map_poke_run+0xc2/0x34e [ 402.824744] kasan_report.cold+0x7c/0xd8 [ 402.824752] ? prog_array_map_poke_run+0xc2/0x34e [ 402.824757] prog_array_map_poke_run+0xc2/0x34e [ 402.824765] bpf_fd_array_map_update_elem+0x124/0x1a0 The elements concerned are walked like this, for (i = 0; i < elem->aux->size_poke_tab; i++) { poke = &elem->aux->poke_tab[i]; So the access to size_poke_tab is the 4B read, verified by checking offsets in the KASAN dump, [ 402.825004] The buggy address belongs to the object at ffff8881905a7800 which belongs to the cache kmalloc-1k of size 1024 [ 402.825008] The buggy address is located 320 bytes inside of 1024-byte region [ffff8881905a7800, ffff8881905a7c00) With pahol output, struct bpf_prog_aux { ... /* --- cacheline 5 boundary (320 bytes) --- */ u32 size_poke_tab; /* 320 4 */ ... To fix track the map references by using a per subprogram used_maps array and used_map_cnt values to hold references into the maps so when the subprogram is released we can then untrack from the correct map using the correct aux field. Here we a slightly less than optimal because we insert all poke entries into the used_map array, even duplicates. In theory we could get by with only unique entries. This would require an extra collect the maps stage though and seems unnecessary when this is simply an extra 8B per duplicate. It also makes the logic simpler and the untrack hook is happy to ignore entries previously removed. Reported-by: Jussi Maki <[email protected]> Signed-off-by: John Fastabend <[email protected]>
1 parent 384d7d4 commit 0657a91

File tree

3 files changed

+30
-13
lines changed

3 files changed

+30
-13
lines changed

include/linux/bpf.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1780,6 +1780,7 @@ static inline struct bpf_prog *bpf_prog_get_type(u32 ufd,
17801780
return bpf_prog_get_type_dev(ufd, type, false);
17811781
}
17821782

1783+
void bpf_free_used_maps(struct bpf_prog_aux *aux);
17831784
void __bpf_free_used_maps(struct bpf_prog_aux *aux,
17841785
struct bpf_map **used_maps, u32 len);
17851786

kernel/bpf/core.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2167,7 +2167,7 @@ void __bpf_free_used_maps(struct bpf_prog_aux *aux,
21672167
}
21682168
}
21692169

2170-
static void bpf_free_used_maps(struct bpf_prog_aux *aux)
2170+
void bpf_free_used_maps(struct bpf_prog_aux *aux)
21712171
{
21722172
__bpf_free_used_maps(aux, aux->used_maps, aux->used_map_cnt);
21732173
kfree(aux->used_maps);
@@ -2211,8 +2211,10 @@ static void bpf_prog_free_deferred(struct work_struct *work)
22112211
#endif
22122212
if (aux->dst_trampoline)
22132213
bpf_trampoline_put(aux->dst_trampoline);
2214-
for (i = 0; i < aux->func_cnt; i++)
2214+
for (i = 0; i < aux->func_cnt; i++) {
2215+
bpf_free_used_maps(aux->func[i]->aux);
22152216
bpf_jit_free(aux->func[i]);
2217+
}
22162218
if (aux->func_cnt) {
22172219
kfree(aux->func);
22182220
bpf_prog_unlock_free(aux->prog);

kernel/bpf/verifier.c

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12128,14 +12128,32 @@ static int jit_subprogs(struct bpf_verifier_env *env)
1212812128
func[i]->insnsi[insn_idx - subprog_start].imm = ret + 1;
1212912129
}
1213012130

12131-
for (j = 0; j < func[i]->aux->size_poke_tab; j++) {
12132-
int ret;
12131+
/* overapproximate the number of map slots. Untrack will just skip
12132+
* the lookup anyways and we avoid an extra layer of accounting.
12133+
*/
12134+
if (func[i]->aux->size_poke_tab) {
12135+
struct bpf_map **used_maps;
1213312136

12134-
map_ptr = func[i]->aux->poke_tab[j].tail_call.map;
12135-
ret = map_ptr->ops->map_poke_track(map_ptr, func[i]->aux);
12136-
if (ret < 0) {
12137-
verbose(env, "tracking tail call prog failed\n");
12137+
used_maps = kmalloc_array(func[i]->aux->size_poke_tab,
12138+
sizeof(struct bpf_map *),
12139+
GFP_KERNEL);
12140+
if (!used_maps)
1213812141
goto out_free;
12142+
12143+
func[i]->aux->used_maps = used_maps;
12144+
12145+
for (j = 0; j < func[i]->aux->size_poke_tab; j++) {
12146+
int ret;
12147+
12148+
map_ptr = func[i]->aux->poke_tab[j].tail_call.map;
12149+
ret = map_ptr->ops->map_poke_track(map_ptr, func[i]->aux);
12150+
if (ret < 0) {
12151+
verbose(env, "tracking tail call prog failed\n");
12152+
goto out_free;
12153+
}
12154+
bpf_map_inc(map_ptr);
12155+
func[i]->aux->used_map_cnt++;
12156+
func[i]->aux->used_maps[j] = map_ptr;
1213912157
}
1214012158
}
1214112159

@@ -12259,11 +12277,7 @@ static int jit_subprogs(struct bpf_verifier_env *env)
1225912277
for (i = 0; i < env->subprog_cnt; i++) {
1226012278
if (!func[i])
1226112279
continue;
12262-
12263-
for (j = 0; j < func[i]->aux->size_poke_tab; j++) {
12264-
map_ptr = func[i]->aux->poke_tab[j].tail_call.map;
12265-
map_ptr->ops->map_poke_untrack(map_ptr, func[i]->aux);
12266-
}
12280+
bpf_free_used_maps(func[i]->aux);
1226712281
bpf_jit_free(func[i]);
1226812282
}
1226912283
kfree(func);

0 commit comments

Comments
 (0)