| Message ID | 20220421130056.2510372-1-cuigaosheng1@huawei.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | BPF |
| Headers | show |
| Series | [-next] libbpf: Add additional null-pointer checking in make_parent_dir | expand |
| Context | Check | Description |
|---|---|---|
| bpf/vmtest-bpf-next-PR | success | PR summary |
| bpf/vmtest-bpf-next-VM_Test-2 | success | Logs for Kernel LATEST on z15 + selftests |
| bpf/vmtest-bpf-next-VM_Test-1 | success | Logs for Kernel LATEST on ubuntu-latest + selftests |
| netdev/tree_selection | success | Not a local patch |
On Thu, Apr 21, 2022 at 6:01 AM Gaosheng Cui <cuigaosheng1@huawei.com> wrote: > > The make_parent_dir is called without null-pointer checking for path, > such as bpf_link__pin. To ensure there is no null-pointer dereference > in make_parent_dir, so make_parent_dir requires additional null-pointer > checking for path. > > Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > --- > tools/lib/bpf/libbpf.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index b53e51884f9e..5786e6184bf5 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -7634,6 +7634,9 @@ static int make_parent_dir(const char *path) > char *dname, *dir; > int err = 0; > > + if (path == NULL) > + return -EINVAL; > + API contract is that path shouldn't be NULL. Just like we don't check link, obj, prog for NULL in every single API, I don't think we need to do it here, unless I'm missing something? > dname = strdup(path); > if (dname == NULL) > return -ENOMEM; > -- > 2.25.1 >
I don't understand why we don't check path for NULL, bpf_link__pin is an external interface, It will be called by external functions and provide input parameters, for example in samples/bpf/hbm.c: > 201 link = bpf_program__attach_cgroup(bpf_prog, cg1); 202 if > (libbpf_get_error(link)) { 203 fprintf(stderr, "ERROR: > bpf_program__attach_cgroup failed\n"); 204 goto err; 205 } 206 207 > sprintf(cg_pin_path, "/sys/fs/bpf/hbm%d", cg_id); 208 rc = > bpf_link__pin(link, cg_pin_path); 209 if (rc < 0) { 210 printf("ERROR: > bpf_link__pin failed: %d\n", rc); 211 goto err; 212 } if cg_pin_path is NULL, strdup(NULL) will trigger a segmentation fault in make_parent_dir, I think we should avoid this and add null-pointer checking for path, just like check_path: > 7673 static int check_path(const char *path) 7674 { 7675 char *cp, > errmsg[STRERR_BUFSIZE]; 7676 struct statfs st_fs; 7677 char *dname, > *dir; 7678 int err = 0; 7679 7680 if (path == NULL) 7681 return > -EINVAL; 7682 7683 dname = strdup(path); 7684 if (dname == NULL) 7685 > return -ENOMEM; 7686 7687 dir = dirname(dname); 7688 if (statfs(dir, > &st_fs)) { 7689 cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg)); > 7690 pr_warn("failed to statfs %s: %s\n", dir, cp); 7691 err = -errno; > 7692 } 7693 free(dname); 7694 7695 if (!err && st_fs.f_type != > BPF_FS_MAGIC) { 7696 pr_warn("specified path %s is not on BPF FS\n", > path); 7697 err = -EINVAL; 7698 } 7699 7700 return err; 7701 } Thanks. 在 2022/4/22 0:55, Andrii Nakryiko 写道: > On Thu, Apr 21, 2022 at 6:01 AM Gaosheng Cui <cuigaosheng1@huawei.com> wrote: >> The make_parent_dir is called without null-pointer checking for path, >> such as bpf_link__pin. To ensure there is no null-pointer dereference >> in make_parent_dir, so make_parent_dir requires additional null-pointer >> checking for path. >> >> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >> --- >> tools/lib/bpf/libbpf.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index b53e51884f9e..5786e6184bf5 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -7634,6 +7634,9 @@ static int make_parent_dir(const char *path) >> char *dname, *dir; >> int err = 0; >> >> + if (path == NULL) >> + return -EINVAL; >> + > API contract is that path shouldn't be NULL. Just like we don't check > link, obj, prog for NULL in every single API, I don't think we need to > do it here, unless I'm missing something? > >> dname = strdup(path); >> if (dname == NULL) >> return -ENOMEM; >> -- >> 2.25.1 >> > .
This email adjusts the code format. I don't understand why we don't check path for NULL, bpf_link__pin is an external interface, It will be called by external functions and provide input parameters, for example in samples/bpf/hbm.c: > 201 link = bpf_program__attach_cgroup(bpf_prog, cg1); > 202 if (libbpf_get_error(link)) { > 203 fprintf(stderr, "ERROR: bpf_program__attach_cgroup > failed\n"); > 204 goto err; > 205 } > 206 > 207 sprintf(cg_pin_path, "/sys/fs/bpf/hbm%d", cg_id); > 208 rc = bpf_link__pin(link, cg_pin_path); > 209 if (rc < 0) { > 210 printf("ERROR: bpf_link__pin failed: %d\n", rc); > 211 goto err; > 212 } if cg_pin_path is NULL, strdup(NULL) will trigger a segmentation fault in make_parent_dir, I think we should avoid this and add null-pointer checking for path, just like check_path: > 7673 static int check_path(const char *path) > 7674 { > 7675 char *cp, errmsg[STRERR_BUFSIZE]; > 7676 struct statfs st_fs; > 7677 char *dname, *dir; > 7678 int err = 0; > 7679 > 7680 if (path == NULL) > 7681 return -EINVAL; > 7682 > 7683 dname = strdup(path); > 7684 if (dname == NULL) > 7685 return -ENOMEM; > 7686 > 7687 dir = dirname(dname); > 7688 if (statfs(dir, &st_fs)) { > 7689 cp = libbpf_strerror_r(errno, errmsg, > sizeof(errmsg)); > 7690 pr_warn("failed to statfs %s: %s\n", dir, cp); > 7691 err = -errno; > 7692 } > 7693 free(dname); > 7694 > 7695 if (!err && st_fs.f_type != BPF_FS_MAGIC) { > 7696 pr_warn("specified path %s is not on BPF FS\n", > path); > 7697 err = -EINVAL; > 7698 } > 7699 > 7700 return err; > 7701 } Thanks. 在 2022/4/22 0:55, Andrii Nakryiko 写道: > On Thu, Apr 21, 2022 at 6:01 AM Gaosheng Cui <cuigaosheng1@huawei.com> wrote: >> The make_parent_dir is called without null-pointer checking for path, >> such as bpf_link__pin. To ensure there is no null-pointer dereference >> in make_parent_dir, so make_parent_dir requires additional null-pointer >> checking for path. >> >> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> >> --- >> tools/lib/bpf/libbpf.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index b53e51884f9e..5786e6184bf5 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -7634,6 +7634,9 @@ static int make_parent_dir(const char *path) >> char *dname, *dir; >> int err = 0; >> >> + if (path == NULL) >> + return -EINVAL; >> + > API contract is that path shouldn't be NULL. Just like we don't check > link, obj, prog for NULL in every single API, I don't think we need to > do it here, unless I'm missing something? > >> dname = strdup(path); >> if (dname == NULL) >> return -ENOMEM; >> -- >> 2.25.1 >> > .
On Thu, Apr 21, 2022 at 7:55 PM cuigaosheng <cuigaosheng1@huawei.com> wrote: > > This email adjusts the code format. > > I don't understand why we don't check path for NULL, bpf_link__pin is an > external > interface, It will be called by external functions and provide input > parameters, that external interface expects non-NULL string as input argument, which is a default throughout libbpf's API. You will get SIGSEGV in lots of cases if you pass NULL where you are not supposed to, e.g., bpf_object__open_file() and many others. It doesn't mean that libbpf should check any pointer argument for NULL. You can argue that strdup(NULL) shouldn't crash but it doesn't. It's because strdup() has a contract that it shouldn't be passed NULL. So is the case here. > for example in samples/bpf/hbm.c: > > > 201 link = bpf_program__attach_cgroup(bpf_prog, cg1); > > 202 if (libbpf_get_error(link)) { > > 203 fprintf(stderr, "ERROR: bpf_program__attach_cgroup > > failed\n"); > > 204 goto err; > > 205 } > > 206 > > 207 sprintf(cg_pin_path, "/sys/fs/bpf/hbm%d", cg_id); > > 208 rc = bpf_link__pin(link, cg_pin_path); > > 209 if (rc < 0) { > > 210 printf("ERROR: bpf_link__pin failed: %d\n", rc); > > 211 goto err; > > 212 } > > if cg_pin_path is NULL, strdup(NULL) will trigger a segmentation fault in > make_parent_dir, I think we should avoid this and add null-pointer checking > for path, just like check_path: > > 7673 static int check_path(const char *path) > > 7674 { > > 7675 char *cp, errmsg[STRERR_BUFSIZE]; > > 7676 struct statfs st_fs; > > 7677 char *dname, *dir; > > 7678 int err = 0; > > 7679 > > 7680 if (path == NULL) > > 7681 return -EINVAL; > > 7682 > > 7683 dname = strdup(path); > > 7684 if (dname == NULL) > > 7685 return -ENOMEM; > > 7686 > > 7687 dir = dirname(dname); > > 7688 if (statfs(dir, &st_fs)) { > > 7689 cp = libbpf_strerror_r(errno, errmsg, > > sizeof(errmsg)); > > 7690 pr_warn("failed to statfs %s: %s\n", dir, cp); > > 7691 err = -errno; > > 7692 } > > 7693 free(dname); > > 7694 > > 7695 if (!err && st_fs.f_type != BPF_FS_MAGIC) { > > 7696 pr_warn("specified path %s is not on BPF FS\n", > > path); > > 7697 err = -EINVAL; > > 7698 } > > 7699 > > 7700 return err; > > 7701 } > > Thanks. > > > 在 2022/4/22 0:55, Andrii Nakryiko 写道: > > On Thu, Apr 21, 2022 at 6:01 AM Gaosheng Cui <cuigaosheng1@huawei.com> wrote: > >> The make_parent_dir is called without null-pointer checking for path, > >> such as bpf_link__pin. To ensure there is no null-pointer dereference > >> in make_parent_dir, so make_parent_dir requires additional null-pointer > >> checking for path. > >> > >> Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> > >> --- > >> tools/lib/bpf/libbpf.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index b53e51884f9e..5786e6184bf5 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -7634,6 +7634,9 @@ static int make_parent_dir(const char *path) > >> char *dname, *dir; > >> int err = 0; > >> > >> + if (path == NULL) > >> + return -EINVAL; > >> + > > API contract is that path shouldn't be NULL. Just like we don't check > > link, obj, prog for NULL in every single API, I don't think we need to > > do it here, unless I'm missing something? > > > >> dname = strdup(path); > >> if (dname == NULL) > >> return -ENOMEM; > >> -- > >> 2.25.1 > >> > > .
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index b53e51884f9e..5786e6184bf5 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -7634,6 +7634,9 @@ static int make_parent_dir(const char *path) char *dname, *dir; int err = 0; + if (path == NULL) + return -EINVAL; + dname = strdup(path); if (dname == NULL) return -ENOMEM;
The make_parent_dir is called without null-pointer checking for path, such as bpf_link__pin. To ensure there is no null-pointer dereference in make_parent_dir, so make_parent_dir requires additional null-pointer checking for path. Signed-off-by: Gaosheng Cui <cuigaosheng1@huawei.com> --- tools/lib/bpf/libbpf.c | 3 +++ 1 file changed, 3 insertions(+)