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

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

[Jamal Hadi Salim]

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

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, 9 insertions(+), 8 deletions(-)

-- 
2.20.1

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

[Jamal Hadi Salim]

From: Jamal Hadi Salim <hadi@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 | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/lib/bpf.c b/lib/bpf.c
index 10cf9bf4..656cad02 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1509,15 +1509,15 @@ 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));
+	ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
 	if (ret < 0) {
-		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
-		goto out;
+		fprintf(stderr, "snprintf failed: %s\n", strerror(errno));
+		return ret;
 	}
 
 	ret = asprintf(&rem, "%s/", todo);
@@ -1547,7 +1547,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

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

[Andrea Claudi]

On Wed, Apr 22, 2020 at 12:28 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> From: Jamal Hadi Salim <hadi@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 | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 10cf9bf4..656cad02 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1509,15 +1509,15 @@ 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));
> +       ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));

Shouldn't we check for the last argument length before? We should have
enough space for "/../" and the terminator, so we need the last
argument length to be less than PATH_MAX-5, right?


>         if (ret < 0) {
> -               fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
> -               goto out;
> +               fprintf(stderr, "snprintf failed: %s\n", strerror(errno));
> +               return ret;
>         }
>
>         ret = asprintf(&rem, "%s/", todo);
> @@ -1547,7 +1547,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
>

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

[Dominique Martinet]

Andrea Claudi wrote on Wed, Apr 22, 2020:
> > -       ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> > +       ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
> 
> Shouldn't we check for the last argument length before? We should have
> enough space for "/../" and the terminator, so we need the last
> argument length to be less than PATH_MAX-5, right?

snprintf will return the length that would be written if there had been
room so most codes just check the return value instead (something like
if (ret >= sizeof(tmp)) error)

OTOH this will actually be caught by the later strcat guard, because rem
will always contain at least on / the while will always be entered and
strlen(tmp) + (>=0) + 2 will always be > PATH_MAX, so this function will
error out.
I'll admit it's not clear, though :)

I'm actually not sure snprintf can return < 0... wide character
formatting functions can but basic ones not really, there's hardly any
snprintf return checking in iproute2 code...


Anyway, with all that said this patch currently technically works for
me, despite being not so clear, so can add my reviewed-by on whatever
final version you take (check < 0 or >= PATH_MAX or none or both), if
you'd like :)

-- 
Dominique

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

[Jamal Hadi Salim]

On 2020-04-22 10:43 a.m., Dominique Martinet wrote:

[...]
> I'm actually not sure snprintf can return < 0...

Man page says it can (quick grep on iproute2 code shows some
invocations can check for <=0)

> wide character
> formatting functions can but basic ones not really, there's hardly any
> snprintf return checking in iproute2 code...
> 
> 
> Anyway, with all that said this patch currently technically works for
> me, despite being not so clear, so can add my reviewed-by on whatever
> final version you take (check < 0 or >= PATH_MAX or none or both), if
> you'd like :)

  Will update the next chance i get to.

 >
 > Now I'm looking at this we're a bit inconsistent with return values in
 > this function, other error paths return negative value + errno set,
 > and this only returns -errno.
 >
 > Digging a bit into callers it looks like errno is the way to go, so
 > while you're modifying this it might be worth setting errno
 > to EINVALas
 > well here?
 >

Will do. I wanted to leave code that didnt affect me alone; but
granted it is reasonable for consistency..

 >
 > Cheers & sorry for nitpicking,
 >

No sweat - we havent yet entered the realm  where the color
of the bike shed  begins to matter ;->

cheers,
jamal

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

[Stephen Hemminger]

On Wed, 22 Apr 2020 06:28:07 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

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

This is not a sufficient commit message. You need to describe what the
problem is and why this fixes it.


> 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, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 10cf9bf4..656cad02 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1509,15 +1509,15 @@ out:
>  static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
>  				const char *todo)
>  {
> -	char *tmp = NULL;
> +	char tmp[PATH_MAX] = {};

Initializing the whole string to 0 is over kill here.

>  	char *rem = NULL;
>  	char *sub;
>  	int ret;
>  
> -	ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> +	ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));

snprintf will never return -1.

>  	if (ret < 0) {
> -		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
> -		goto out;
> +		fprintf(stderr, "snprintf failed: %s\n", strerror(errno));
> +		return ret;
>  	}
>  
>  	ret = asprintf(&rem, "%s/", todo);
> @@ -1547,7 +1547,6 @@ static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
>  	ret = 0;
>  out:
>  	free(rem);
> -	free(tmp);
>  	return ret;
>  }
>  

This patch needs to be reworked.

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

[Jamal Hadi Salim]

On 2020-04-22 12:35 p.m., Stephen Hemminger wrote:
> On Wed, 22 Apr 2020 06:28:07 -0400
> Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> 
>> From: Jamal Hadi Salim <hadi@mojatatu.com>
>>
> 
> This is not a sufficient commit message. You need to describe what the
> problem is and why this fixes it.
> 
> 
>> 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, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/bpf.c b/lib/bpf.c
>> index 10cf9bf4..656cad02 100644
>> --- a/lib/bpf.c
>> +++ b/lib/bpf.c
>> @@ -1509,15 +1509,15 @@ out:
>>   static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
>>   				const char *todo)
>>   {
>> -	char *tmp = NULL;
>> +	char tmp[PATH_MAX] = {};
> 
> Initializing the whole string to 0 is over kill here.

Why is it overkill? ;->
Note: I just replicated other parts of the same file which
initialize similar array to 0.

> 
>>   	char *rem = NULL;
>>   	char *sub;
>>   	int ret;
>>   
>> -	ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
>> +	ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
> 
> snprintf will never return -1.

Man page says it does. Practically it may not but we have code (in
iproute2) which assumes it will happen.

Pick your poison:
1) Ignore the return code
2) As suggested by Dominique, something along the lines of:
if (ret <= 0 || ret >= MAX_PATH)
    ...error here..

Which one do you prefer?

cheers,
jamal

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

[Dominique Martinet]

Jamal Hadi Salim wrote on Wed, Apr 22, 2020:
> >>diff --git a/lib/bpf.c b/lib/bpf.c
> >>index 10cf9bf4..656cad02 100644
> >>--- a/lib/bpf.c
> >>+++ b/lib/bpf.c
> >>@@ -1509,15 +1509,15 @@ out:
> >>  static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
> >>  				const char *todo)
> >>  {
> >>-	char *tmp = NULL;
> >>+	char tmp[PATH_MAX] = {};
> >
> >Initializing the whole string to 0 is over kill here.
> 
> Why is it overkill? ;->
> Note: I just replicated other parts of the same file which
> initialize similar array to 0.

FWIW I kind of agree this is overkill, there's only one other occurence
of a char * being explicitely zeroed, the rest isn't strings so probably
have better reasons to.

snprintf will safely zero-terminate it and nothing should ever access
past the nul byte so it shouldn't be necessary.

> >>  	char *rem = NULL;
> >>  	char *sub;
> >>  	int ret;
> >>-	ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> >>+	ret = snprintf(tmp, PATH_MAX, "%s/../", bpf_get_work_dir(ctx->type));
> >
> >snprintf will never return -1.
> 
> Man page says it does. Practically it may not but we have code (in
> iproute2) which assumes it will happen.
> 
> Pick your poison:
> 1) Ignore the return code
> 2) As suggested by Dominique, something along the lines of:

(I also said I don't think it can ever fail in the non-wide-char variant
we use here (failure described in man page might be bad format string?
but we use a constant string here), and that the >= check is redundant
with the later strcat boundary checking ; by the same logic the words
you put in my mouth here are overkill as well :) (and the max size
variant would need some extra andling to set errno so check cannot be
shared that easily)
Anyway rest of iproute2 doesn't check snprintf return value much, it
should be fine to ignore)

> if (ret <= 0 || ret >= MAX_PATH)
>    ...error here..
> 
> Which one do you prefer?

-- 
Dominique

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

[Jamal Hadi Salim]

On 2020-04-23 2:30 a.m., Dominique Martinet wrote:
> Jamal Hadi Salim wrote on Wed, Apr 22, 2020:

[..]
>>> Initializing the whole string to 0 is over kill here.
>>
>> Why is it overkill? ;->
>> Note: I just replicated other parts of the same file which
>> initialize similar array to 0.
> 
> FWIW I kind of agree this is overkill, there's only one other occurence
> of a char * being explicitely zeroed, the rest isn't strings so probably
> have better reasons to.

But "overkill" is such a strong word;-> Is it affecting performance,
readability, or is the point that it was over-engineering exercise?
Do note: there's other code that does the same thing (even in
the same file!). And all i did was, in TheLinuxWay(tm),
just cutnpasted.
Same thing with checking for return code of snprintf.

Consistency is the problem - because there are different styles
in the same tree. Possibly had i sent the code using the suggested
approach someone would have probably pointed out i shouldve used
the approach i did;->
In any case, i think we are now heading in the direction
of the bike shed ;->

I will fix this and snprintf in the next update.

> 
> snprintf will safely zero-terminate it and nothing should ever access
> past the nul byte so it shouldn't be necessary.
> 

It would also depend on the knowledge of the coder on what could
go wrong.
You may still want to know what you think you wrote in there was
picked up (so checking max size)..
I dont know why the manpage says you'll get a negative return
but the safest thing against Murphy should be to check for all
possible boundary conditions.

Note: The original fix from Andrea was for a compiler warning on
snprintf.

I will send the next update..

cheers,
jamal

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

[Jamal Hadi Salim]

From: Jamal Hadi Salim <hadi@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 656cad02..fdcede1c 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -1523,13 +1523,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) {
+			ret = -EINVAL;
+			goto out;
+		}
 
 		strcat(tmp, sub);
 		strcat(tmp, "/");
-- 
2.20.1

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

[Dominique Martinet]

Jamal Hadi Salim wrote on Wed, Apr 22, 2020:
>  	sub = strtok(rem, "/");
>  	while (sub) {
> -		if (strlen(tmp) + strlen(sub) + 2 > PATH_MAX)
> -			return -EINVAL;
> +		if (strlen(tmp) + strlen(sub) + 2 > PATH_MAX) {
> +			ret = -EINVAL;
> +			goto out;
> +		}

Now I'm looking at this we're a bit inconsistent with return values in
this function, other error paths return negative value + errno set,
and this only returns -errno.

Digging a bit into callers it looks like errno is the way to go, so
while you're modifying this it might be worth setting errno to EINVAL as
well here?


Cheers & sorry for nitpicking,
-- 
Dominique

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

[Stephen Hemminger]

On Wed, 22 Apr 2020 06:28:06 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:

> From: Jamal Hadi Salim <hadi@mojatatu.com>
> 
> 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, 9 insertions(+), 8 deletions(-)
> 

Would be good to see a v3 and ideally ACK's from of the
other BPF developers such as Daniel Borkmann
or Jakub Kiciniski