diff mbox series

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

Message ID 20200421180426.6945-1-jhs@emojatatu.com
State Changes Requested
Delegated to: stephen hemminger
Headers show
Series [iproute2,1/1] bpf: Fix segfault when custom pinning is used | expand

Commit Message

Jamal Hadi Salim April 21, 2020, 6:04 p.m. UTC 
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 | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Andrea Claudi April 21, 2020, 7:38 p.m. UTC | #1 
On Tue, Apr 21, 2020 at 8:05 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 | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 10cf9bf4..cf636c9e 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1509,12 +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));
> +       ret = sprintf(tmp, "%s/../", bpf_get_work_dir(ctx->type));
>         if (ret < 0) {
>                 fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
>                 goto out;
> @@ -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
>

Hi Jamal,
Are you sure this fixes your issue? I think asprintf could segfault
only if ctx is null, but this case is not addressed in your patch.
I remember that Stephen asked me to use asprintf to avoid allocating
huge buffers on stack; anyway I've no objection to sprintf, if needed.

Andrea
Jamal Hadi Salim April 21, 2020, 7:58 p.m. UTC | #2 
Hi Andrea,

On 2020-04-21 3:38 p.m., Andrea Claudi wrote:
[..]
> 
> Hi Jamal,
> Are you sure this fixes your issue?

Yes.

> I think asprintf could segfault
> only if ctx is null, but this case is not addressed in your patch.

The issue is tmp(after it is created by asprintf) gets trampled.
This:
ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
allocates enough space for tmp.
But then later:
strcat(tmp, sub);
strcat(tmp...);
creates a buffer overrun.

It is easy to overlook things like these when making a large
semantically-equivalent change - so i would suggest to also
review the general patch that went from sprintf->asprintf.

> I remember that Stephen asked me to use asprintf to avoid allocating
> huge buffers on stack; anyway I've no objection to sprintf, if needed.

Generally that is good practise. And even for this case
you could probably find a simpler way to solve it with asprintf
or realloc trickery. I am not sure it is worth the bother - 4K on
the stack in user space is not a big deal really.

cheers,
jamal
Andrea Claudi April 21, 2020, 10:10 p.m. UTC | #3 
On Tue, Apr 21, 2020 at 9:58 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> Hi Andrea,
>
> On 2020-04-21 3:38 p.m., Andrea Claudi wrote:
> [..]
> >
> > Hi Jamal,
> > Are you sure this fixes your issue?
>
> Yes.
>
> > I think asprintf could segfault
> > only if ctx is null, but this case is not addressed in your patch.
>
> The issue is tmp(after it is created by asprintf) gets trampled.
> This:
> ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
> allocates enough space for tmp.
> But then later:
> strcat(tmp, sub);
> strcat(tmp...);
> creates a buffer overrun.
>
> It is easy to overlook things like these when making a large
> semantically-equivalent change - so i would suggest to also
> review the general patch that went from sprintf->asprintf.
>

Oh, now I see. Thanks for pointing it out and making it clear to me.
I agree with you, this needs to be carefully reviewed to ensure we are
not falling into the same error pattern somewhere else.

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

> > I remember that Stephen asked me to use asprintf to avoid allocating
> > huge buffers on stack; anyway I've no objection to sprintf, if needed.
>
> Generally that is good practise. And even for this case
> you could probably find a simpler way to solve it with asprintf
> or realloc trickery. I am not sure it is worth the bother - 4K on
> the stack in user space is not a big deal really.

Stephen, what do you think about using sprintf instead of asprintf in
these functions?
When dealing with paths, asprintf can indeed be a bit error-prone. I
can easily imagine this error pattern to happen again in the future.
If you agree, I can send a patch taking care of this.

> cheers,
> jamal
>

Andrea
Dominique Martinet April 22, 2020, 6:42 a.m. UTC | #4 
(random review)

Jamal Hadi Salim wrote on Tue, Apr 21, 2020:
> diff --git a/lib/bpf.c b/lib/bpf.c
> index 10cf9bf4..cf636c9e 100644
> --- a/lib/bpf.c
> +++ b/lib/bpf.c
> @@ -1509,12 +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));
> +	ret = sprintf(tmp, "%s/../", bpf_get_work_dir(ctx->type));
>  	if (ret < 0) {
>  		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));

error check needs to be reworded, and it probably needs to use snprintf
instead of sprintf: bpf_get_work_dir() can be up to PATH_MAX long and as
pointed out there are strcat() afterwards so it's still possible to
overflow this one
Jamal Hadi Salim April 22, 2020, 9:47 a.m. UTC | #5 
On 2020-04-22 2:42 a.m., Dominique Martinet wrote:
> (random review)
> 

Good review;->

> Jamal Hadi Salim wrote on Tue, Apr 21, 2020:
>> diff --git a/lib/bpf.c b/lib/bpf.c
>> index 10cf9bf4..cf636c9e 100644
>> --- a/lib/bpf.c
>> +++ b/lib/bpf.c
>> @@ -1509,12 +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));
>> +	ret = sprintf(tmp, "%s/../", bpf_get_work_dir(ctx->type));
>>   	if (ret < 0) {
>>   		fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
> 
> error check needs to be reworded,

Will reword.

  and it probably needs to use snprintf
> instead of sprintf: bpf_get_work_dir() can be up to PATH_MAX long and as
> pointed out there are strcat() afterwards so it's still possible to
> overflow this one
> 

and change to snprintf. The strcat afterwards is protected by the check
                 if (strlen(tmp) + strlen(sub) + 2 > PATH_MAX)
                         return -EINVAL;
However, since you looked at it now that i am paying closer attention
there's another bug there. The above check will return without freeing
earlier asprintf allocated "rem" variable. Sounds like separate patch.

cheers,
jamal
diff mbox series

Patch

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