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 |
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
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
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
(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
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 --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; }