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@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.
>
> > 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;
> >
>
