| Message ID | 20230609221637.2631800-1-eddyz87@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v1] selftests/bpf: fix invalid pointer check in get_xlated_program() | expand |
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.) > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > 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@gmail.com> > --- > 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. > if (bpf_prog_get_info_by_fd(fd_prog, &info, &info_len)) { > perror("bpf_prog_get_info_by_fd failed"); > - return -1; > + return NULL; > } > > xlated_prog_len = info.xlated_prog_len; > if (xlated_prog_len % buf_element_size) { > printf("Program length %d is not multiple of %d\n", > xlated_prog_len, buf_element_size); > - return -1; > + return NULL; > } > > *cnt = xlated_prog_len / buf_element_size; > - *buf = calloc(*cnt, buf_element_size); > + buf = calloc(*cnt, buf_element_size); > if (!buf) { > perror("can't allocate xlated program buffer"); > - return -ENOMEM; > + return NULL; > } > > bzero(&info, sizeof(info)); > info.xlated_prog_len = xlated_prog_len; > - info.xlated_prog_insns = (__u64)(unsigned long)*buf; > + info.xlated_prog_insns = (__u64)(unsigned long)buf; > if (bpf_prog_get_info_by_fd(fd_prog, &info, &info_len)) { > perror("second bpf_prog_get_info_by_fd failed"); > goto out_free_buf; > } > > - return 0; > + return buf; > > out_free_buf: > - free(*buf); > - return -1; > + free(buf); > + return NULL; > } > > static bool is_null_insn(struct bpf_insn *insn) > @@ -1512,7 +1515,8 @@ static bool check_xlated_program(struct bpf_test *test, int fd_prog) > if (!check_expected && !check_unexpected) > goto out; > > - if (get_xlated_program(fd_prog, &buf, &cnt)) { > + buf = get_xlated_program(fd_prog, &cnt); > + if (!buf) { > printf("FAIL: can't get xlated program\n"); > result = false; > goto out; >
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). But I can rollback to a small fix if you insist. > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > > 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@gmail.com> > > --- > > 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. > > > if (bpf_prog_get_info_by_fd(fd_prog, &info, &info_len)) { > > perror("bpf_prog_get_info_by_fd failed"); > > - return -1; > > + return NULL; > > } > > > > xlated_prog_len = info.xlated_prog_len; > > if (xlated_prog_len % buf_element_size) { > > printf("Program length %d is not multiple of %d\n", > > xlated_prog_len, buf_element_size); > > - return -1; > > + return NULL; > > } > > > > *cnt = xlated_prog_len / buf_element_size; > > - *buf = calloc(*cnt, buf_element_size); > > + buf = calloc(*cnt, buf_element_size); > > if (!buf) { > > perror("can't allocate xlated program buffer"); > > - return -ENOMEM; > > + return NULL; > > } > > > > bzero(&info, sizeof(info)); > > info.xlated_prog_len = xlated_prog_len; > > - info.xlated_prog_insns = (__u64)(unsigned long)*buf; > > + info.xlated_prog_insns = (__u64)(unsigned long)buf; > > if (bpf_prog_get_info_by_fd(fd_prog, &info, &info_len)) { > > perror("second bpf_prog_get_info_by_fd failed"); > > goto out_free_buf; > > } > > > > - return 0; > > + return buf; > > > > out_free_buf: > > - free(*buf); > > - return -1; > > + free(buf); > > + return NULL; > > } > > > > static bool is_null_insn(struct bpf_insn *insn) > > @@ -1512,7 +1515,8 @@ static bool check_xlated_program(struct bpf_test *test, int fd_prog) > > if (!check_expected && !check_unexpected) > > goto out; > > > > - if (get_xlated_program(fd_prog, &buf, &cnt)) { > > + buf = get_xlated_program(fd_prog, &cnt); > > + if (!buf) { > > printf("FAIL: can't get xlated program\n"); > > result = false; > > goto out; > > >
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@linaro.org> >>> 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@gmail.com> >>> --- >>> 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!
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); if (bpf_prog_get_info_by_fd(fd_prog, &info, &info_len)) { perror("bpf_prog_get_info_by_fd failed"); - return -1; + return NULL; } xlated_prog_len = info.xlated_prog_len; if (xlated_prog_len % buf_element_size) { printf("Program length %d is not multiple of %d\n", xlated_prog_len, buf_element_size); - return -1; + return NULL; } *cnt = xlated_prog_len / buf_element_size; - *buf = calloc(*cnt, buf_element_size); + buf = calloc(*cnt, buf_element_size); if (!buf) { perror("can't allocate xlated program buffer"); - return -ENOMEM; + return NULL; } bzero(&info, sizeof(info)); info.xlated_prog_len = xlated_prog_len; - info.xlated_prog_insns = (__u64)(unsigned long)*buf; + info.xlated_prog_insns = (__u64)(unsigned long)buf; if (bpf_prog_get_info_by_fd(fd_prog, &info, &info_len)) { perror("second bpf_prog_get_info_by_fd failed"); goto out_free_buf; } - return 0; + return buf; out_free_buf: - free(*buf); - return -1; + free(buf); + return NULL; } static bool is_null_insn(struct bpf_insn *insn) @@ -1512,7 +1515,8 @@ static bool check_xlated_program(struct bpf_test *test, int fd_prog) if (!check_expected && !check_unexpected) goto out; - if (get_xlated_program(fd_prog, &buf, &cnt)) { + buf = get_xlated_program(fd_prog, &cnt); + if (!buf) { printf("FAIL: can't get xlated program\n"); result = false; goto out;
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. Reported-by: Dan Carpenter <dan.carpenter@linaro.org> 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@gmail.com> --- tools/testing/selftests/bpf/test_verifier.c | 26 ++++++++++++--------- 1 file changed, 15 insertions(+), 11 deletions(-)