[PATCH iproute2 v3 0/2] bpf: memory access fixes

[PATCH iproute2 v3 0/2] bpf: memory access fixes

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Changes from V2:
 1) Dont initialize tmp on stack (Stephen)
 2) Dont look at the return code of snprintf (Dominique)
 3) Set errno to EINVAL instead of returning -EINVAL for consistency (Dominique)

Changes from V1:
 1) use snprintf instead of sprintf and fix corresponding error message.
 Caught-by: Dominique Martinet <asmadeus@codewreck.org>
 2) Fix memory leak and extraneous free() in error path

Jamal Hadi Salim (2):
  bpf: Fix segfault when custom pinning is used
  bpf: Fix mem leak and extraneous free() in error path

 lib/bpf.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

-- 
2.20.1

[PATCH iproute2 v3 1/2] bpf: Fix segfault when custom pinning is used

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

How to recreate:
1) Create a custome pinned map - example something along
   the lines of:

   struct bpf_elf_map SEC("maps") my_map = {
        .type = BPF_MAP_TYPE_HASH,
        .size_key = sizeof(struct my_key),
        .size_value = sizeof(struct my_value),
        .pinning = 6,
        .max_elem = 16,
   };

2) load the program with tc filter and tc will segfault.

The reason is we strcat past memory allocated using asprintf.
Solution - just use a static buffer of max possible size of 4k.

Fixes: c0325b06382 ("bpf: replace snprintf with asprintf when dealing with long buffers")

Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 lib/bpf.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/lib/bpf.c b/lib/bpf.c
index 10cf9bf4..73f3a590 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1509,16 +1509,12 @@ out:
 static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
 				const char *todo)
 {
-	char *tmp = NULL;
+	char tmp[PATH_MAX];
 	char *rem = NULL;
 	char *sub;
 	int ret;
 
-	ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
-	if (ret < 0) {
-		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
-		goto out;
-	}
+	snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
 
 	ret = asprintf(&rem, "%s/", todo);
 	if (ret < 0) {
@@ -1547,7 +1543,6 @@ static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
 	ret = 0;
 out:
 	free(rem);
-	free(tmp);
 	return ret;
 }
 
-- 
2.20.1

[PATCH iproute2 v3 2/2] bpf: Fix mem leak and extraneous free() in error path

[Jamal Hadi Salim]

From: Jamal Hadi Salim <jhs@mojatatu.com>

Fixes: c0325b06382 ("bpf: replace snprintf with asprintf when dealing with long buffers")
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
 lib/bpf.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/bpf.c b/lib/bpf.c
index 73f3a590..b05c8568 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1519,13 +1519,15 @@ static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
 	ret = asprintf(&rem, "%s/", todo);
 	if (ret < 0) {
 		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
-		goto out;
+		return ret;
 	}
 
 	sub = strtok(rem, "/");
 	while (sub) {
-		if (strlen(tmp) + strlen(sub) + 2 > PATH_MAX)
-			return -EINVAL;
+		if (strlen(tmp) + strlen(sub) + 2 > PATH_MAX) {
+			errno = EINVAL;
+			goto out;
+		}
 
 		strcat(tmp, sub);
 		strcat(tmp, "/");
-- 
2.20.1

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Andrea Claudi]

On Thu, Apr 23, 2020 at 7:59 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> From: Jamal Hadi Salim <jhs@mojatatu.com>
>
> Changes from V2:
>  1) Dont initialize tmp on stack (Stephen)
>  2) Dont look at the return code of snprintf (Dominique)
>  3) Set errno to EINVAL instead of returning -EINVAL for consistency (Dominique)
>
> Changes from V1:
>  1) use snprintf instead of sprintf and fix corresponding error message.
>  Caught-by: Dominique Martinet <asmadeus@codewreck.org>
>  2) Fix memory leak and extraneous free() in error path
>
> Jamal Hadi Salim (2):
>   bpf: Fix segfault when custom pinning is used
>   bpf: Fix mem leak and extraneous free() in error path
>
>  lib/bpf.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
>
> --
> 2.20.1
>

Acked-by: Andrea Claudi <aclaudi@redhat.com>

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Jamal Hadi Salim]

Stephen,
What happened to this?

cheers,
jamal

On 2020-04-23 1:58 p.m., Jamal Hadi Salim wrote:
> From: Jamal Hadi Salim <jhs@mojatatu.com>
> 
> Changes from V2:
>   1) Dont initialize tmp on stack (Stephen)
>   2) Dont look at the return code of snprintf (Dominique)
>   3) Set errno to EINVAL instead of returning -EINVAL for consistency (Dominique)
> 
> Changes from V1:
>   1) use snprintf instead of sprintf and fix corresponding error message.
>   Caught-by: Dominique Martinet <asmadeus@codewreck.org>
>   2) Fix memory leak and extraneous free() in error path
> 
> Jamal Hadi Salim (2):
>    bpf: Fix segfault when custom pinning is used
>    bpf: Fix mem leak and extraneous free() in error path
> 
>   lib/bpf.c | 17 +++++++----------
>   1 file changed, 7 insertions(+), 10 deletions(-)
> 

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Jamal Hadi Salim]

ping?

Note: these are trivial bug fixes.

cheers,
jamal

On 2020-04-28 12:15 p.m., Jamal Hadi Salim wrote:
> Stephen,
> What happened to this?
> 
> cheers,
> jamal
> 
> On 2020-04-23 1:58 p.m., Jamal Hadi Salim wrote:
>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>
>> Changes from V2:
>>   1) Dont initialize tmp on stack (Stephen)
>>   2) Dont look at the return code of snprintf (Dominique)
>>   3) Set errno to EINVAL instead of returning -EINVAL for consistency 
>> (Dominique)
>>
>> Changes from V1:
>>   1) use snprintf instead of sprintf and fix corresponding error message.
>>   Caught-by: Dominique Martinet <asmadeus@codewreck.org>
>>   2) Fix memory leak and extraneous free() in error path
>>
>> Jamal Hadi Salim (2):
>>    bpf: Fix segfault when custom pinning is used
>>    bpf: Fix mem leak and extraneous free() in error path
>>
>>   lib/bpf.c | 17 +++++++----------
>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>
> 

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Daniel Borkmann]

On 5/18/20 3:00 PM, Jamal Hadi Salim wrote:
> ping?
> 
> Note: these are trivial bug fixes.

Looking at c0325b06382c ("bpf: replace snprintf with asprintf when dealing with long buffers"),
I wonder whether it's best to just revert and redo cleanly from scratch.. How much testing has
been performed on the original patch? We know it is causing regressions, and looking Jamal's
2nd patch we do have patterns all over the place wrt error path that go like:

   +	char *file = NULL;
   +	char buff[4096];
  	FILE *fp;
   +	int ret;

   -	snprintf(file, sizeof(file), "/proc/%d/fdinfo/%d", getpid(), fd);
   +	ret = asprintf(&file, "/proc/%d/fdinfo/%d", getpid(), fd);
   +	if (ret < 0) {
   +		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
   +		free(file);
   +		return ret;
   +	}

The man page on asprintf(char **strp, ...) says: "When successful, these functions return
the number of bytes printed, just like sprintf(3). If memory allocation wasn't possible,
or some other error occurs, these functions will return -1, and the contents of strp are
undefined." What is the rationale that are we passing it to free() /everywhere/ in error
path when the API spec does say it's undefined? It may happen to work but file's value
could just as well be, say, 42 ...

Thanks,
Daniel

> cheers,
> jamal
> 
> On 2020-04-28 12:15 p.m., Jamal Hadi Salim wrote:
>> Stephen,
>> What happened to this?
>>
>> cheers,
>> jamal
>>
>> On 2020-04-23 1:58 p.m., Jamal Hadi Salim wrote:
>>> From: Jamal Hadi Salim <jhs@mojatatu.com>
>>>
>>> Changes from V2:
>>>   1) Dont initialize tmp on stack (Stephen)
>>>   2) Dont look at the return code of snprintf (Dominique)
>>>   3) Set errno to EINVAL instead of returning -EINVAL for consistency (Dominique)
>>>
>>> Changes from V1:
>>>   1) use snprintf instead of sprintf and fix corresponding error message.
>>>   Caught-by: Dominique Martinet <asmadeus@codewreck.org>
>>>   2) Fix memory leak and extraneous free() in error path
>>>
>>> Jamal Hadi Salim (2):
>>>    bpf: Fix segfault when custom pinning is used
>>>    bpf: Fix mem leak and extraneous free() in error path
>>>
>>>   lib/bpf.c | 17 +++++++----------
>>>   1 file changed, 7 insertions(+), 10 deletions(-)
>>>
>>
> 

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Jamal Hadi Salim]

On 2020-05-22 9:33 p.m., Daniel Borkmann wrote:
> On 5/18/20 3:00 PM, Jamal Hadi Salim wrote:
>> ping?
>>
>> Note: these are trivial bug fixes.
> 
> Looking at c0325b06382c ("bpf: replace snprintf with asprintf when 
> dealing with long buffers"),
> I wonder whether it's best to just revert and redo cleanly from 
> scratch.. How much testing has
> been performed on the original patch? We know it is causing regressions, 
> and looking Jamal's
> 2nd patch we do have patterns all over the place wrt error path that go 
> like:

Reverting c0325b06382c would work as well..

Note: I believe Andrea's original goal was to just get rid of a
compiler warning from sprintf(). Stephen suggested to use
asprintf. Andrea's original solution to get rid of the compiler
warning would suffice. Maybe then an additional code audit to
ensure consistency on usage of s[n]printf could be done and
resolved separately.

Thanks for taking the time Daniel.

cheers,
jamal

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Andrea Claudi]

On Sat, May 23, 2020 at 12:32 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On 2020-05-22 9:33 p.m., Daniel Borkmann wrote:
> > On 5/18/20 3:00 PM, Jamal Hadi Salim wrote:
> >> ping?
> >>
> >> Note: these are trivial bug fixes.
> >
> > Looking at c0325b06382c ("bpf: replace snprintf with asprintf when
> > dealing with long buffers"),
> > I wonder whether it's best to just revert and redo cleanly from
> > scratch.. How much testing has
> > been performed on the original patch? We know it is causing regressions,
> > and looking Jamal's
> > 2nd patch we do have patterns all over the place wrt error path that go
> > like:
>
> Reverting c0325b06382c would work as well..
>
> Note: I believe Andrea's original goal was to just get rid of a
> compiler warning from sprintf(). Stephen suggested to use
> asprintf. Andrea's original solution to get rid of the compiler
> warning would suffice. Maybe then an additional code audit to
> ensure consistency on usage of s[n]printf could be done and
> resolved separately.
>

Reverting c0325b06382c will for sure fix the segfault identified by
Jamal and get rid of the problems highlighted by Daniel and others.
To fix the s[n]printf truncation warning we can simply check for its
return value. From the snprintf man page:

"a return value of size or more means that the output was truncated."
(caveat: until glibc 2.0.6 ret value for truncation is -1)

Jamal: if this works for you, I can submit an alternative to this
patch series doing what I proposed above. What do you think?

Regards,
Andrea

> Thanks for taking the time Daniel.
>
> cheers,
> jamal
>

Re: [PATCH iproute2 v3 0/2] bpf: memory access fixes

[Jamal Hadi Salim]

On 2020-05-25 4:53 a.m., Andrea Claudi wrote:


> 
> Reverting c0325b06382c will for sure fix the segfault identified by
> Jamal and get rid of the problems highlighted by Daniel and others.
> To fix the s[n]printf truncation warning we can simply check for its
> return value. From the snprintf man page:
> 
> "a return value of size or more means that the output was truncated."
> (caveat: until glibc 2.0.6 ret value for truncation is -1)
> 
> Jamal: if this works for you, I can submit an alternative to this
> patch series doing what I proposed above. What do you think?
> 

I am ok with that approach.

cheers,
jamal