* [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c
@ 2020-05-26 16:04 Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 1/2] Revert "bpf: replace snprintf with asprintf when dealing with long buffers" Andrea Claudi
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Andrea Claudi @ 2020-05-26 16:04 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern, jhs
Jamal reported a segfault in bpf_make_custom_path() when custom pinning is
used. This is caused by commit c0325b06382cb ("bpf: replace snprintf with
asprintf when dealing with long buffers").
As the only goal of that commit is to get rid of a truncation warning when
compiling lib/bpf.c, revert it and fix the warning checking for snprintf
return value
Andrea Claudi (2):
Revert "bpf: replace snprintf with asprintf when dealing with long
buffers"
bpf: Fixes a snprintf truncation warning
lib/bpf.c | 155 +++++++++++++++---------------------------------------
1 file changed, 41 insertions(+), 114 deletions(-)
--
2.25.4
^ permalink raw reply [flat|nested] 6+ messages in thread
* [iproute2 PATCH 1/2] Revert "bpf: replace snprintf with asprintf when dealing with long buffers"
2020-05-26 16:04 [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Andrea Claudi
@ 2020-05-26 16:04 ` Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 2/2] bpf: Fixes a snprintf truncation warning Andrea Claudi
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Andrea Claudi @ 2020-05-26 16:04 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern, jhs
This reverts commit c0325b06382cb4f7ebfaf80c29c8800d74666fd9.
It introduces a segfault in bpf_make_custom_path() when custom pinning is used.
This happens because asprintf allocates exactly the space needed to hold a
string in the buffer passed as its first argument, but if this buffer is later
used in strcat() or similar we have a buffer overrun.
As the aim of commit c0325b06382c is simply to fix a compiler warning, it
seems safe and reasonable to revert it.
Fixes: c0325b06382c ("bpf: replace snprintf with asprintf when dealing with long buffers")
Reported-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
lib/bpf.c | 155 ++++++++++++++----------------------------------------
1 file changed, 39 insertions(+), 116 deletions(-)
diff --git a/lib/bpf.c b/lib/bpf.c
index 10cf9bf44419a..23cb0d96a85ba 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -406,21 +406,13 @@ static int bpf_derive_elf_map_from_fdinfo(int fd, struct bpf_elf_map *map,
struct bpf_map_ext *ext)
{
unsigned int val, owner_type = 0, owner_jited = 0;
- char *file = NULL;
- char buff[4096];
+ char file[PATH_MAX], buff[4096];
FILE *fp;
- int ret;
- ret = asprintf(&file, "/proc/%d/fdinfo/%d", getpid(), fd);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- free(file);
- return ret;
- }
+ snprintf(file, sizeof(file), "/proc/%d/fdinfo/%d", getpid(), fd);
memset(map, 0, sizeof(*map));
fp = fopen(file, "r");
- free(file);
if (!fp) {
fprintf(stderr, "No procfs support?!\n");
return -EIO;
@@ -608,9 +600,8 @@ int bpf_trace_pipe(void)
0,
};
int fd_in, fd_out = STDERR_FILENO;
- char *tpipe = NULL;
+ char tpipe[PATH_MAX];
const char *mnt;
- int ret;
mnt = bpf_find_mntpt("tracefs", TRACEFS_MAGIC, tracefs_mnt,
sizeof(tracefs_mnt), tracefs_known_mnts);
@@ -619,15 +610,9 @@ int bpf_trace_pipe(void)
return -1;
}
- ret = asprintf(&tpipe, "%s/trace_pipe", mnt);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- free(tpipe);
- return ret;
- }
+ snprintf(tpipe, sizeof(tpipe), "%s/trace_pipe", mnt);
fd_in = open(tpipe, O_RDONLY);
- free(tpipe);
if (fd_in < 0)
return -1;
@@ -648,50 +633,37 @@ int bpf_trace_pipe(void)
static int bpf_gen_global(const char *bpf_sub_dir)
{
- char *bpf_glo_dir = NULL;
+ char bpf_glo_dir[PATH_MAX];
int ret;
- ret = asprintf(&bpf_glo_dir, "%s/%s/", bpf_sub_dir, BPF_DIR_GLOBALS);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
+ snprintf(bpf_glo_dir, sizeof(bpf_glo_dir), "%s/%s/",
+ bpf_sub_dir, BPF_DIR_GLOBALS);
ret = mkdir(bpf_glo_dir, S_IRWXU);
if (ret && errno != EEXIST) {
fprintf(stderr, "mkdir %s failed: %s\n", bpf_glo_dir,
strerror(errno));
- goto out;
+ return ret;
}
- ret = 0;
-out:
- free(bpf_glo_dir);
- return ret;
+ return 0;
}
static int bpf_gen_master(const char *base, const char *name)
{
- char *bpf_sub_dir = NULL;
+ char bpf_sub_dir[PATH_MAX + NAME_MAX + 1];
int ret;
- ret = asprintf(&bpf_sub_dir, "%s%s/", base, name);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
+ snprintf(bpf_sub_dir, sizeof(bpf_sub_dir), "%s%s/", base, name);
ret = mkdir(bpf_sub_dir, S_IRWXU);
if (ret && errno != EEXIST) {
fprintf(stderr, "mkdir %s failed: %s\n", bpf_sub_dir,
strerror(errno));
- goto out;
+ return ret;
}
- ret = bpf_gen_global(bpf_sub_dir);
-out:
- free(bpf_sub_dir);
- return ret;
+ return bpf_gen_global(bpf_sub_dir);
}
static int bpf_slave_via_bind_mnt(const char *full_name,
@@ -720,22 +692,13 @@ static int bpf_slave_via_bind_mnt(const char *full_name,
static int bpf_gen_slave(const char *base, const char *name,
const char *link)
{
- char *bpf_lnk_dir = NULL;
- char *bpf_sub_dir = NULL;
+ char bpf_lnk_dir[PATH_MAX + NAME_MAX + 1];
+ char bpf_sub_dir[PATH_MAX + NAME_MAX];
struct stat sb = {};
int ret;
- ret = asprintf(&bpf_lnk_dir, "%s%s/", base, link);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
-
- ret = asprintf(&bpf_sub_dir, "%s%s", base, name);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
+ snprintf(bpf_lnk_dir, sizeof(bpf_lnk_dir), "%s%s/", base, link);
+ snprintf(bpf_sub_dir, sizeof(bpf_sub_dir), "%s%s", base, name);
ret = symlink(bpf_lnk_dir, bpf_sub_dir);
if (ret) {
@@ -743,30 +706,25 @@ static int bpf_gen_slave(const char *base, const char *name,
if (errno != EPERM) {
fprintf(stderr, "symlink %s failed: %s\n",
bpf_sub_dir, strerror(errno));
- goto out;
+ return ret;
}
- ret = bpf_slave_via_bind_mnt(bpf_sub_dir, bpf_lnk_dir);
- goto out;
+ return bpf_slave_via_bind_mnt(bpf_sub_dir,
+ bpf_lnk_dir);
}
ret = lstat(bpf_sub_dir, &sb);
if (ret) {
fprintf(stderr, "lstat %s failed: %s\n",
bpf_sub_dir, strerror(errno));
- goto out;
+ return ret;
}
- if ((sb.st_mode & S_IFMT) != S_IFLNK) {
- ret = bpf_gen_global(bpf_sub_dir);
- goto out;
- }
+ if ((sb.st_mode & S_IFMT) != S_IFLNK)
+ return bpf_gen_global(bpf_sub_dir);
}
-out:
- free(bpf_lnk_dir);
- free(bpf_sub_dir);
- return ret;
+ return 0;
}
static int bpf_gen_hierarchy(const char *base)
@@ -784,7 +742,7 @@ static int bpf_gen_hierarchy(const char *base)
static const char *bpf_get_work_dir(enum bpf_prog_type type)
{
static char bpf_tmp[PATH_MAX] = BPF_DIR_MNT;
- static char *bpf_wrk_dir;
+ static char bpf_wrk_dir[PATH_MAX];
static const char *mnt;
static bool bpf_mnt_cached;
const char *mnt_env = getenv(BPF_ENV_MNT);
@@ -823,12 +781,7 @@ static const char *bpf_get_work_dir(enum bpf_prog_type type)
}
}
- ret = asprintf(&bpf_wrk_dir, "%s/", mnt);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- free(bpf_wrk_dir);
- goto out;
- }
+ snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
ret = bpf_gen_hierarchy(bpf_wrk_dir);
if (ret) {
@@ -1485,48 +1438,31 @@ static int bpf_probe_pinned(const char *name, const struct bpf_elf_ctx *ctx,
static int bpf_make_obj_path(const struct bpf_elf_ctx *ctx)
{
- char *tmp = NULL;
+ char tmp[PATH_MAX];
int ret;
- ret = asprintf(&tmp, "%s/%s", bpf_get_work_dir(ctx->type), ctx->obj_uid);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
+ snprintf(tmp, sizeof(tmp), "%s/%s", bpf_get_work_dir(ctx->type),
+ ctx->obj_uid);
ret = mkdir(tmp, S_IRWXU);
if (ret && errno != EEXIST) {
fprintf(stderr, "mkdir %s failed: %s\n", tmp, strerror(errno));
- goto out;
+ return ret;
}
- ret = 0;
-out:
- free(tmp);
- return ret;
+ return 0;
}
static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
const char *todo)
{
- char *tmp = NULL;
- char *rem = NULL;
- char *sub;
+ char tmp[PATH_MAX], rem[PATH_MAX], *sub;
int ret;
- ret = asprintf(&tmp, "%s/../", bpf_get_work_dir(ctx->type));
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
-
- ret = asprintf(&rem, "%s/", todo);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- goto out;
- }
-
+ snprintf(tmp, sizeof(tmp), "%s/../", bpf_get_work_dir(ctx->type));
+ snprintf(rem, sizeof(rem), "%s/", todo);
sub = strtok(rem, "/");
+
while (sub) {
if (strlen(tmp) + strlen(sub) + 2 > PATH_MAX)
return -EINVAL;
@@ -1538,17 +1474,13 @@ static int bpf_make_custom_path(const struct bpf_elf_ctx *ctx,
if (ret && errno != EEXIST) {
fprintf(stderr, "mkdir %s failed: %s\n", tmp,
strerror(errno));
- goto out;
+ return ret;
}
sub = strtok(NULL, "/");
}
- ret = 0;
-out:
- free(rem);
- free(tmp);
- return ret;
+ return 0;
}
static int bpf_place_pinned(int fd, const char *name,
@@ -2655,23 +2587,14 @@ struct bpf_jited_aux {
static int bpf_derive_prog_from_fdinfo(int fd, struct bpf_prog_data *prog)
{
- char *file = NULL;
- char buff[4096];
+ char file[PATH_MAX], buff[4096];
unsigned int val;
FILE *fp;
- int ret;
-
- ret = asprintf(&file, "/proc/%d/fdinfo/%d", getpid(), fd);
- if (ret < 0) {
- fprintf(stderr, "asprintf failed: %s\n", strerror(errno));
- free(file);
- return ret;
- }
+ snprintf(file, sizeof(file), "/proc/%d/fdinfo/%d", getpid(), fd);
memset(prog, 0, sizeof(*prog));
fp = fopen(file, "r");
- free(file);
if (!fp) {
fprintf(stderr, "No procfs support?!\n");
return -EIO;
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [iproute2 PATCH 2/2] bpf: Fixes a snprintf truncation warning
2020-05-26 16:04 [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 1/2] Revert "bpf: replace snprintf with asprintf when dealing with long buffers" Andrea Claudi
@ 2020-05-26 16:04 ` Andrea Claudi
2020-05-26 21:27 ` [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Daniel Borkmann
2020-05-27 22:13 ` Stephen Hemminger
3 siblings, 0 replies; 6+ messages in thread
From: Andrea Claudi @ 2020-05-26 16:04 UTC (permalink / raw)
To: netdev; +Cc: stephen, dsahern, jhs
gcc v9.3.1 reports:
bpf.c: In function ‘bpf_get_work_dir’:
bpf.c:784:49: warning: ‘snprintf’ output may be truncated before the last format character [-Wformat-truncation=]
784 | snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
| ^
bpf.c:784:2: note: ‘snprintf’ output between 2 and 4097 bytes into a destination of size 4096
784 | snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Fix this simply checking snprintf return code and properly handling the error.
Fixes: e42256699cac ("bpf: make tc's bpf loader generic and move into lib")
Signed-off-by: Andrea Claudi <aclaudi@redhat.com>
---
lib/bpf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/lib/bpf.c b/lib/bpf.c
index 23cb0d96a85ba..c7d45077c14e5 100644
--- a/lib/bpf.c
+++ b/lib/bpf.c
@@ -781,7 +781,11 @@ static const char *bpf_get_work_dir(enum bpf_prog_type type)
}
}
- snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
+ ret = snprintf(bpf_wrk_dir, sizeof(bpf_wrk_dir), "%s/", mnt);
+ if (ret < 0 || ret >= sizeof(bpf_wrk_dir)) {
+ mnt = NULL;
+ goto out;
+ }
ret = bpf_gen_hierarchy(bpf_wrk_dir);
if (ret) {
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c
2020-05-26 16:04 [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 1/2] Revert "bpf: replace snprintf with asprintf when dealing with long buffers" Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 2/2] bpf: Fixes a snprintf truncation warning Andrea Claudi
@ 2020-05-26 21:27 ` Daniel Borkmann
2020-05-27 22:13 ` Stephen Hemminger
3 siblings, 0 replies; 6+ messages in thread
From: Daniel Borkmann @ 2020-05-26 21:27 UTC (permalink / raw)
To: Andrea Claudi, netdev; +Cc: stephen, dsahern, jhs
On 5/26/20 6:04 PM, Andrea Claudi wrote:
> Jamal reported a segfault in bpf_make_custom_path() when custom pinning is
> used. This is caused by commit c0325b06382cb ("bpf: replace snprintf with
> asprintf when dealing with long buffers").
>
> As the only goal of that commit is to get rid of a truncation warning when
> compiling lib/bpf.c, revert it and fix the warning checking for snprintf
> return value
>
> Andrea Claudi (2):
> Revert "bpf: replace snprintf with asprintf when dealing with long
> buffers"
> bpf: Fixes a snprintf truncation warning
>
> lib/bpf.c | 155 +++++++++++++++---------------------------------------
> 1 file changed, 41 insertions(+), 114 deletions(-)
>
Thanks for following up, Andrea! For the two:
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c
2020-05-26 16:04 [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Andrea Claudi
` (2 preceding siblings ...)
2020-05-26 21:27 ` [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Daniel Borkmann
@ 2020-05-27 22:13 ` Stephen Hemminger
2020-05-28 11:14 ` Jamal Hadi Salim
3 siblings, 1 reply; 6+ messages in thread
From: Stephen Hemminger @ 2020-05-27 22:13 UTC (permalink / raw)
To: Andrea Claudi; +Cc: netdev, dsahern, jhs
On Tue, 26 May 2020 18:04:09 +0200
Andrea Claudi <aclaudi@redhat.com> wrote:
> Jamal reported a segfault in bpf_make_custom_path() when custom pinning is
> used. This is caused by commit c0325b06382cb ("bpf: replace snprintf with
> asprintf when dealing with long buffers").
>
> As the only goal of that commit is to get rid of a truncation warning when
> compiling lib/bpf.c, revert it and fix the warning checking for snprintf
> return value
>
> Andrea Claudi (2):
> Revert "bpf: replace snprintf with asprintf when dealing with long
> buffers"
> bpf: Fixes a snprintf truncation warning
>
> lib/bpf.c | 155 +++++++++++++++---------------------------------------
> 1 file changed, 41 insertions(+), 114 deletions(-)
>
ok merged
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c
2020-05-27 22:13 ` Stephen Hemminger
@ 2020-05-28 11:14 ` Jamal Hadi Salim
0 siblings, 0 replies; 6+ messages in thread
From: Jamal Hadi Salim @ 2020-05-28 11:14 UTC (permalink / raw)
To: Stephen Hemminger, Andrea Claudi; +Cc: netdev, dsahern, asmadeus
On 2020-05-27 6:13 p.m., Stephen Hemminger wrote:
> On Tue, 26 May 2020 18:04:09 +0200
> Andrea Claudi <aclaudi@redhat.com> wrote:
>
>> Jamal reported a segfault in bpf_make_custom_path() when custom pinning is
>> used. This is caused by commit c0325b06382cb ("bpf: replace snprintf with
>> asprintf when dealing with long buffers").
>>
>> As the only goal of that commit is to get rid of a truncation warning when
>> compiling lib/bpf.c, revert it and fix the warning checking for snprintf
>> return value
>>
>> Andrea Claudi (2):
>> Revert "bpf: replace snprintf with asprintf when dealing with long
>> buffers"
>> bpf: Fixes a snprintf truncation warning
>>
>> lib/bpf.c | 155 +++++++++++++++---------------------------------------
>> 1 file changed, 41 insertions(+), 114 deletions(-)
>>
>
> ok merged
>
FWIW, it may be useful to grep the tree and check for
s[n]printf() return code.
It seems like modern compilers are good enough at catching
overruns but maybe useful to enforce a coding style consistency
given that most people doit the LinuxWay (cutnpaste existing
code to fix a bug or add a feature).
cheers,
jamal
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-28 11:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-26 16:04 [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 1/2] Revert "bpf: replace snprintf with asprintf when dealing with long buffers" Andrea Claudi
2020-05-26 16:04 ` [iproute2 PATCH 2/2] bpf: Fixes a snprintf truncation warning Andrea Claudi
2020-05-26 21:27 ` [iproute2 PATCH 0/2] Fix segfault in lib/bpf.c Daniel Borkmann
2020-05-27 22:13 ` Stephen Hemminger
2020-05-28 11:14 ` Jamal Hadi Salim
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).