[PATCH bpf 0/3] samples/bpf: syscall_tp_user: Refactor and fix array index out-of-bounds bug

[PATCH bpf 0/3] samples/bpf: syscall_tp_user: Refactor and fix array index out-of-bounds bug

[Jinghao Jia]

There are currently 6 BPF programs in syscall_tp_kern but the array to
hold the corresponding bpf_links in syscall_tp_user only has space for 4
programs, given the array size is hardcoded. This causes the sample
program to fail due to an out-of-bound access that corrupts other stack
variables:

  # ./syscall_tp
  prog #0: map ids 4 5
  verify map:4 val: 5
  map_lookup failed: Bad file descriptor

This patch series aims to solve this issue for now and for the future.
It first adds the -fsanitize=bounds flag to make similar bugs more
obvious at runtime. It then refactors syscall_tp_user to retrieve the
number of programs from the bpf_object and dynamically allocate the
array of bpf_links to avoid inconsistencies from hardcoding.

Jinghao Jia (3):
  samples/bpf: Add -fsanitize=bounds to userspace programs
  samples/bpf: syscall_tp_user: Rename num_progs into nr_tests
  samples/bpf: syscall_tp_user: Fix array out-of-bound access

 samples/bpf/Makefile          |  1 +
 samples/bpf/syscall_tp_user.c | 44 ++++++++++++++++++++++++-----------
 2 files changed, 31 insertions(+), 14 deletions(-)

-- 
2.41.0

[PATCH bpf 1/3] samples/bpf: Add -fsanitize=bounds to userspace programs

[Jinghao Jia]

The sanitizer flag, which is supported by both clang and gcc, would make
it easier to debug array index out-of-bounds problems in these programs.

Suggested-by: Mimi Zohar <zohar@linux.ibm.com>
Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com>
---
 samples/bpf/Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 595b98d825ce..340972ed1582 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -193,6 +193,7 @@ endif
 TPROGS_CFLAGS += -Wall -O2
 TPROGS_CFLAGS += -Wmissing-prototypes
 TPROGS_CFLAGS += -Wstrict-prototypes
+TPROGS_CFLAGS += -fsanitize=bounds
 
 TPROGS_CFLAGS += -I$(objtree)/usr/include
 TPROGS_CFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
-- 
2.41.0

[PATCH bpf 2/3] samples/bpf: syscall_tp_user: Rename num_progs into nr_tests

[Jinghao Jia]

The variable name num_progs causes confusion because that variable
really controls the number of rounds the test should be executed.

Rename num_progs into nr_tests for the sake of clarity.

Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com>
---
 samples/bpf/syscall_tp_user.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c
index 7a788bb837fc..18c94c7e8a40 100644
--- a/samples/bpf/syscall_tp_user.c
+++ b/samples/bpf/syscall_tp_user.c
@@ -17,9 +17,9 @@
 
 static void usage(const char *cmd)
 {
-	printf("USAGE: %s [-i num_progs] [-h]\n", cmd);
-	printf("       -i num_progs      # number of progs of the test\n");
-	printf("       -h                # help\n");
+	printf("USAGE: %s [-i nr_tests] [-h]\n", cmd);
+	printf("       -i nr_tests      # rounds of test to run\n");
+	printf("       -h               # help\n");
 }
 
 static void verify_map(int map_id)
@@ -45,14 +45,14 @@ static void verify_map(int map_id)
 	}
 }
 
-static int test(char *filename, int num_progs)
+static int test(char *filename, int nr_tests)
 {
-	int map0_fds[num_progs], map1_fds[num_progs], fd, i, j = 0;
-	struct bpf_link *links[num_progs * 4];
-	struct bpf_object *objs[num_progs];
+	int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0;
+	struct bpf_link *links[nr_tests * 4];
+	struct bpf_object *objs[nr_tests];
 	struct bpf_program *prog;
 
-	for (i = 0; i < num_progs; i++) {
+	for (i = 0; i < nr_tests; i++) {
 		objs[i] = bpf_object__open_file(filename, NULL);
 		if (libbpf_get_error(objs[i])) {
 			fprintf(stderr, "opening BPF object file failed\n");
@@ -101,7 +101,7 @@ static int test(char *filename, int num_progs)
 	close(fd);
 
 	/* verify the map */
-	for (i = 0; i < num_progs; i++) {
+	for (i = 0; i < nr_tests; i++) {
 		verify_map(map0_fds[i]);
 		verify_map(map1_fds[i]);
 	}
@@ -117,13 +117,13 @@ static int test(char *filename, int num_progs)
 
 int main(int argc, char **argv)
 {
-	int opt, num_progs = 1;
+	int opt, nr_tests = 1;
 	char filename[256];
 
 	while ((opt = getopt(argc, argv, "i:h")) != -1) {
 		switch (opt) {
 		case 'i':
-			num_progs = atoi(optarg);
+			nr_tests = atoi(optarg);
 			break;
 		case 'h':
 		default:
@@ -134,5 +134,5 @@ int main(int argc, char **argv)
 
 	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
 
-	return test(filename, num_progs);
+	return test(filename, nr_tests);
 }
-- 
2.41.0

[PATCH bpf 3/3] samples/bpf: syscall_tp_user: Fix array out-of-bound access

[Jinghao Jia]

Commit 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint
to syscall_tp sample") added two more eBPF programs to support the
openat2() syscall. However, it did not increase the size of the array
that holds the corresponding bpf_links. This leads to an out-of-bound
access on that array in the bpf_object__for_each_program loop and could
corrupt other variables on the stack. On our testing QEMU, it corrupts
the map1_fds array and causes the sample to fail:

  # ./syscall_tp
  prog #0: map ids 4 5
  verify map:4 val: 5
  map_lookup failed: Bad file descriptor

Dynamically allocate the array based on the number of programs reported
by libbpf to prevent similar inconsistencies in the future

Fixes: 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample")
Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com>
---
 samples/bpf/syscall_tp_user.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c
index 18c94c7e8a40..8855d2c1290d 100644
--- a/samples/bpf/syscall_tp_user.c
+++ b/samples/bpf/syscall_tp_user.c
@@ -48,7 +48,7 @@ static void verify_map(int map_id)
 static int test(char *filename, int nr_tests)
 {
 	int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0;
-	struct bpf_link *links[nr_tests * 4];
+	struct bpf_link **links = NULL;
 	struct bpf_object *objs[nr_tests];
 	struct bpf_program *prog;
 
@@ -60,6 +60,17 @@ static int test(char *filename, int nr_tests)
 			goto cleanup;
 		}
 
+		/* One-time initialization */
+		if (!links) {
+			int nr_progs = 0;
+
+			bpf_object__for_each_program(prog, objs[i])
+				nr_progs += 1;
+
+			links = calloc(nr_progs * nr_tests,
+				       sizeof(struct bpf_link *));
+		}
+
 		/* load BPF program */
 		if (bpf_object__load(objs[i])) {
 			fprintf(stderr, "loading BPF object file failed\n");
@@ -107,8 +118,13 @@ static int test(char *filename, int nr_tests)
 	}
 
 cleanup:
-	for (j--; j >= 0; j--)
-		bpf_link__destroy(links[j]);
+	if (links) {
+		for (j--; j >= 0; j--)
+			bpf_link__destroy(links[j]);
+
+		free(links);
+		links = NULL;
+	}
 
 	for (i--; i >= 0; i--)
 		bpf_object__close(objs[i]);
-- 
2.41.0

Re: [PATCH bpf 3/3] samples/bpf: syscall_tp_user: Fix array out-of-bound access

[Daniel Borkmann]

On 8/18/23 6:46 PM, Jinghao Jia wrote:
> Commit 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint
> to syscall_tp sample") added two more eBPF programs to support the
> openat2() syscall. However, it did not increase the size of the array
> that holds the corresponding bpf_links. This leads to an out-of-bound
> access on that array in the bpf_object__for_each_program loop and could
> corrupt other variables on the stack. On our testing QEMU, it corrupts
> the map1_fds array and causes the sample to fail:
> 
>    # ./syscall_tp
>    prog #0: map ids 4 5
>    verify map:4 val: 5
>    map_lookup failed: Bad file descriptor
> 
> Dynamically allocate the array based on the number of programs reported
> by libbpf to prevent similar inconsistencies in the future
> 
> Fixes: 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample")
> Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com>
> ---
>   samples/bpf/syscall_tp_user.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c
> index 18c94c7e8a40..8855d2c1290d 100644
> --- a/samples/bpf/syscall_tp_user.c
> +++ b/samples/bpf/syscall_tp_user.c
> @@ -48,7 +48,7 @@ static void verify_map(int map_id)
>   static int test(char *filename, int nr_tests)
>   {
>   	int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0;
> -	struct bpf_link *links[nr_tests * 4];
> +	struct bpf_link **links = NULL;
>   	struct bpf_object *objs[nr_tests];
>   	struct bpf_program *prog;
>   
> @@ -60,6 +60,17 @@ static int test(char *filename, int nr_tests)
>   			goto cleanup;
>   		}
>   
> +		/* One-time initialization */
> +		if (!links) {
> +			int nr_progs = 0;
> +
> +			bpf_object__for_each_program(prog, objs[i])
> +				nr_progs += 1;
> +
> +			links = calloc(nr_progs * nr_tests,
> +				       sizeof(struct bpf_link *));

NULL check is missing

> +		}
> +
>   		/* load BPF program */
>   		if (bpf_object__load(objs[i])) {
>   			fprintf(stderr, "loading BPF object file failed\n");
> @@ -107,8 +118,13 @@ static int test(char *filename, int nr_tests)
>   	}
>   
>   cleanup:
> -	for (j--; j >= 0; j--)
> -		bpf_link__destroy(links[j]);
> +	if (links) {
> +		for (j--; j >= 0; j--)
> +			bpf_link__destroy(links[j]);
> +
> +		free(links);
> +		links = NULL;

why is this explicit links = NULL needed?

> +	}
>   
>   	for (i--; i >= 0; i--)
>   		bpf_object__close(objs[i]);
> 

Re: [PATCH bpf 3/3] samples/bpf: syscall_tp_user: Fix array out-of-bound access

[Jinghao Jia]

[-- Attachment #1.1: Type: text/plain, Size: 3293 bytes --]


On 8/25/23 09:57, Daniel Borkmann wrote:
> On 8/18/23 6:46 PM, Jinghao Jia wrote:
>> Commit 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint
>> to syscall_tp sample") added two more eBPF programs to support the
>> openat2() syscall. However, it did not increase the size of the array
>> that holds the corresponding bpf_links. This leads to an out-of-bound
>> access on that array in the bpf_object__for_each_program loop and could
>> corrupt other variables on the stack. On our testing QEMU, it corrupts
>> the map1_fds array and causes the sample to fail:
>>
>>    # ./syscall_tp
>>    prog #0: map ids 4 5
>>    verify map:4 val: 5
>>    map_lookup failed: Bad file descriptor
>>
>> Dynamically allocate the array based on the number of programs reported
>> by libbpf to prevent similar inconsistencies in the future
>>
>> Fixes: 06744f24696e ("samples/bpf: Add openat2() enter/exit tracepoint to syscall_tp sample")
>> Signed-off-by: Jinghao Jia <jinghao@linux.ibm.com>
>> ---
>>   samples/bpf/syscall_tp_user.c | 22 +++++++++++++++++++---
>>   1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/samples/bpf/syscall_tp_user.c b/samples/bpf/syscall_tp_user.c
>> index 18c94c7e8a40..8855d2c1290d 100644
>> --- a/samples/bpf/syscall_tp_user.c
>> +++ b/samples/bpf/syscall_tp_user.c
>> @@ -48,7 +48,7 @@ static void verify_map(int map_id)
>>   static int test(char *filename, int nr_tests)
>>   {
>>       int map0_fds[nr_tests], map1_fds[nr_tests], fd, i, j = 0;
>> -    struct bpf_link *links[nr_tests * 4];
>> +    struct bpf_link **links = NULL;
>>       struct bpf_object *objs[nr_tests];
>>       struct bpf_program *prog;
>>   @@ -60,6 +60,17 @@ static int test(char *filename, int nr_tests)
>>               goto cleanup;
>>           }
>>   +        /* One-time initialization */
>> +        if (!links) {
>> +            int nr_progs = 0;
>> +
>> +            bpf_object__for_each_program(prog, objs[i])
>> +                nr_progs += 1;
>> +
>> +            links = calloc(nr_progs * nr_tests,
>> +                       sizeof(struct bpf_link *));
>
> NULL check is missing

Oh, apparently I missed that, will fix in v2.


>
>> +        }
>> +
>>           /* load BPF program */
>>           if (bpf_object__load(objs[i])) {
>>               fprintf(stderr, "loading BPF object file failed\n");
>> @@ -107,8 +118,13 @@ static int test(char *filename, int nr_tests)
>>       }
>>     cleanup:
>> -    for (j--; j >= 0; j--)
>> -        bpf_link__destroy(links[j]);
>> +    if (links) {
>> +        for (j--; j >= 0; j--)
>> +            bpf_link__destroy(links[j]);
>> +
>> +        free(links);
>> +        links = NULL;
>
> why is this explicit links = NULL needed?

Yeah I agree this is redundant, since links is not used later.


>
>> +    }
>>         for (i--; i >= 0; i--)
>>           bpf_object__close(objs[i]);
>>
>
>
>

I wonder if there are further comments before I roll out a v2?

--Jinghao


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]