* [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used
@ 2020-04-21 18:04 Jamal Hadi Salim
2020-04-21 19:38 ` Andrea Claudi
2020-04-22 6:42 ` Dominique Martinet
0 siblings, 2 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2020-04-21 18:04 UTC (permalink / raw)
To: stephen
Cc: netdev, dsahern, aclaudi, daniel, Jamal Hadi Salim, 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 | 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used
2020-04-21 18:04 [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used Jamal Hadi Salim
@ 2020-04-21 19:38 ` Andrea Claudi
2020-04-21 19:58 ` Jamal Hadi Salim
2020-04-22 6:42 ` Dominique Martinet
1 sibling, 1 reply; 6+ messages in thread
From: Andrea Claudi @ 2020-04-21 19:38 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Stephen Hemminger, linux-netdev, David Ahern, daniel, Jamal Hadi Salim
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used
2020-04-21 19:38 ` Andrea Claudi
@ 2020-04-21 19:58 ` Jamal Hadi Salim
2020-04-21 22:10 ` Andrea Claudi
0 siblings, 1 reply; 6+ messages in thread
From: Jamal Hadi Salim @ 2020-04-21 19:58 UTC (permalink / raw)
To: Andrea Claudi
Cc: Stephen Hemminger, linux-netdev, David Ahern, daniel, Jamal Hadi Salim
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used
2020-04-21 19:58 ` Jamal Hadi Salim
@ 2020-04-21 22:10 ` Andrea Claudi
0 siblings, 0 replies; 6+ messages in thread
From: Andrea Claudi @ 2020-04-21 22:10 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Stephen Hemminger, linux-netdev, David Ahern, daniel, Jamal Hadi Salim
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used
2020-04-21 18:04 [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used Jamal Hadi Salim
2020-04-21 19:38 ` Andrea Claudi
@ 2020-04-22 6:42 ` Dominique Martinet
2020-04-22 9:47 ` Jamal Hadi Salim
1 sibling, 1 reply; 6+ messages in thread
From: Dominique Martinet @ 2020-04-22 6:42 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: stephen, netdev, dsahern, aclaudi, daniel, Jamal Hadi Salim
(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
--
Dominique
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used
2020-04-22 6:42 ` Dominique Martinet
@ 2020-04-22 9:47 ` Jamal Hadi Salim
0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2020-04-22 9:47 UTC (permalink / raw)
To: Dominique Martinet
Cc: stephen, netdev, dsahern, aclaudi, daniel, Jamal Hadi Salim
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-04-22 9:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-21 18:04 [PATCH iproute2 1/1] bpf: Fix segfault when custom pinning is used Jamal Hadi Salim
2020-04-21 19:38 ` Andrea Claudi
2020-04-21 19:58 ` Jamal Hadi Salim
2020-04-21 22:10 ` Andrea Claudi
2020-04-22 6:42 ` Dominique Martinet
2020-04-22 9:47 ` Jamal Hadi Salim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.