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

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(-)

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
>

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 :)

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.

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

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

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?

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