On 6/12/23 5:05 PM, Eduard Zingerman wrote:
On Mon, 2023-06-12 at 17:00 +0200, Daniel Borkmann wrote:
On 6/10/23 12:16 AM, Eduard Zingerman wrote:
Dan Carpenter reported invalid check for calloc() result in
test_verifier.c:get_xlated_program():
./tools/testing/selftests/bpf/test_verifier.c:1365 get_xlated_program()
warn: variable dereferenced before check 'buf' (see line 1364)
./tools/testing/selftests/bpf/test_verifier.c
1363 *cnt = xlated_prog_len / buf_element_size;
1364 *buf = calloc(*cnt, buf_element_size);
1365 if (!buf) {
This should be if (!*buf) {
1366 perror("can't allocate xlated program buffer");
1367 return -ENOMEM;
This commit refactors the get_xlated_program() to avoid using double
pointer type.
Isn't the small reported fix above sufficient? (Either is fine with me though.)
I think it is less prone to mechanical mistakes without double pointers
(in case if this function would be modified sometimes in the future).
Ok.
But I can rollback to a small fix if you insist.
Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
Closes: https://lore.kernel.org/bpf/ZH7u0hEGVB4MjGZq@moroto/
Fixes: 933ff53191eb ("selftests/bpf: specify expected instructions in test_verifier tests")
Signed-off-by: Eduard Zingerman <eddyz87@xxxxxxxxx>
---
tools/testing/selftests/bpf/test_verifier.c | 26 ++++++++++++---------
1 file changed, 15 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c b/tools/testing/selftests/bpf/test_verifier.c
index 71704a38cac3..c6bc9e26d333 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -1341,45 +1341,48 @@ static bool cmp_str_seq(const char *log, const char *exp)
return true;
}
-static int get_xlated_program(int fd_prog, struct bpf_insn **buf, int *cnt)
+static struct bpf_insn *get_xlated_program(int fd_prog, int *cnt)
{
struct bpf_prog_info info = {};
__u32 info_len = sizeof(info);
+ __u32 buf_element_size;
__u32 xlated_prog_len;
- __u32 buf_element_size = sizeof(struct bpf_insn);
+ struct bpf_insn *buf;
+
+ buf_element_size = sizeof(struct bpf_insn);
Just small nit: the `__u32 buf_element_size = sizeof(struct bpf_insn);` could have
stayed as is.
Moved it to have "inverse Christmas tree" for declarations,
can send V2 undoing this.
Nah, it's fine. Fixed up while applying. Thanks!
