BUG: fix uninitialized value usage in gen_bpf.c

[giuseppe]

it was reported by clang with the option -fsanitize=memory:

Uninitialized bytes in MemcmpInterceptorCommon at offset 0 inside [0x7070000002a0, 56)
==3791089==WARNING: MemorySanitizer: use-of-uninitialized-value
#0 0x482a2c in memcmp (fuzzer+0x482a2c)
#1 0x7fed2f120ebb in _hsh_add src/libseccomp/src/gen_bpf.c:598:9
#2 0x7fed2f121715 in _gen_bpf_action_hsh src/libseccomp/src/gen_bpf.c:796:6
#3 0x7fed2f121a53 in _gen_bpf_node src/libseccomp/src/gen_bpf.c:831:11
#4 0x7fed2f121a53 in _gen_bpf_chain.isra.0 src/libseccomp/src/gen_bpf.c:1072:13
#5 0x7fed2f121f16 in _gen_bpf_chain_lvl_res src/libseccomp/src/gen_bpf.c:977:12
#6 0x7fed2f121c74 in _gen_bpf_chain.isra.0 src/libseccomp/src/gen_bpf.c:1124:12
#7 0x7fed2f12253c in _gen_bpf_syscall src/libseccomp/src/gen_bpf.c:1520:10
#8 0x7fed2f12253c in _gen_bpf_syscalls src/libseccomp/src/gen_bpf.c:1615:18
#9 0x7fed2f12253c in _gen_bpf_arch src/libseccomp/src/gen_bpf.c:1683:7
#10 0x7fed2f12253c in _gen_bpf_build_bpf src/libseccomp/src/gen_bpf.c:2056:11
#11 0x7fed2f12253c in gen_bpf_generate src/libseccomp/src/gen_bpf.c:2321:7
#12 0x7fed2f11f41c in seccomp_export_bpf src/libseccomp/src/api.c:724:7

Uninitialized value was created by a heap allocation
#0 0x4547ef in realloc (fuzzer+0x4547ef)
#1 0x7fed2f121244 in _blk_resize src/libseccomp/src/gen_bpf.c:362:8
#2 0x7fed2f121244 in _blk_append src/libseccomp/src/gen_bpf.c:394:6

Signed-off-by: Giuseppe Scrivano [email protected]

[coveralls]

Coverage increased (+0.01%) to 92.936% when pulling 591089f on giuseppe:fix-unitialized-value-usage into c305ef3 on seccomp:main.

[pcmoore]

I could be wrong, but I suspect there is a gap somewhere in struct bpf_blk that isn't normally written to but it is read when _hsh_add_src() runs over the entire struct, triggering the warning/error.

@giuseppe there is already a libseccomp function, zmalloc() in "src/helper.c" which does a malloc() + memset(); how about refactoring your approach in this PR to create a zrealloc() that does a similar thing and then converting the handful of existing calls to realloc()? Of course the zrealloc() implementation would need to take an additional argument to represent the size of the current buffer, but that shouldn't be too hard to sort out.

[giuseppe]

@giuseppe there is already a libseccomp function, zmalloc() in "src/helper.c" which does a malloc() + memset(); how about refactoring your approach in this PR to create a zrealloc() that does a similar thing and then converting the handful of existing calls to realloc()? Of course the zrealloc() implementation would need to take an additional argument to represent the size of the current buffer, but that shouldn't be too hard to sort out.

sure, I can do that.

Are you fine if I refactor zmalloc to use calloc instead of malloc (or possibly just replace zmalloc with calloc)?

AFAIK, calloc avoids the additional memset when the std library knows the memory is already set to 0.

[giuseppe]

pushed a new version that adds zrealloc.

[pcmoore]

This looks much better, thank you! Would you also mind converting the other realloc() callers?

% grep "realloc" $(find ./src -name "*.c")
./src/gen_bpf.c:        new = realloc(blk->blks, blk->blk_alloc * sizeof(*(blk->blks)));
./src/gen_bpf.c:        i_new = realloc(prg->blks, BPF_PGM_SIZE(prg));
./src/db.c:     dbs = realloc(col_dst->filters,
./src/db.c:     dbs = realloc(col->filters,
./src/db.c:             /* NOTE: if we can't do the realloc it isn't fatal, we just
./src/db.c:             dbs = realloc(col->filters,
./src/db.c:             tmp_f = realloc(snap->filters,

Are you fine if I refactor zmalloc to use calloc instead of malloc (or possibly just replace zmalloc with calloc)?

Sure, that sounds good. You can do it in this PR if you like, but please do it in a separate patch from the realloc work.

[giuseppe]

./src/db.c: dbs = realloc(col_dst->filters,
./src/db.c: dbs = realloc(col->filters,
./src/db.c: /* NOTE: if we can't do the realloc it isn't fatal, we just
./src/db.c: dbs = realloc(col->filters,
./src/db.c: tmp_f = realloc(snap->filters,

I've not touched these calls because the new allocated space is immediately written after the realloc, so there won't be any advantage with zrealloc, and it would just be more expensive.

If you still want it, I can change these occurrences as well.

[pcmoore]

I've not touched these calls because the new allocated space is immediately written after the realloc, so there won't be any advantage with zrealloc, and it would just be more expensive.

Fair enough. There could be an argument about doing it anyway for a belt-and-suspenders approach, but looking at the code that is a pretty weak argument in those cases. Let's leave it as-is.

Acked-by: Paul Moore <[email protected]>

What do you think @drakenclimber?

[drakenclimber]

Looks good to me. Thanks, @giuseppe

Acked-by: Tom Hromatka <[email protected]>

[giuseppe]

is this PR fine to merge?

[drakenclimber]

Sorry about that, @giuseppe. I forgot to click the "approve" button. I'll merge it in a second