| Message ID | 20190410003741.1855113-1-yhs@fb.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next,tools/bpf] fix a few ubsan warning | expand |
On 04/10/2019 02:37 AM, Yonghong Song wrote: > The issue is reported at https://github.com/libbpf/libbpf/issues/28. > > Basically, per C standard, for > void *memcpy(void *dest, const void *src, size_t n) > if "dest" or "src" is NULL, regardless of whether "n" is 0 or not, > the result of memcpy is undefined. clang ubsan reported three such > instances in bpf.c with the following pattern: > memcpy(dest, 0, 0). > > Although in practice, no known compiler will cause issues when > copy size is 0. Let us still fix the issue to silence ubsan > warnings. > > Signed-off-by: Yonghong Song <yhs@fb.com> Applied, thanks. I fixed up $SUBJECT while applying to add a subsystem prefix. > --- > tools/lib/bpf/bpf.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c > index a1db869a6fda..78f2400dd2d1 100644 > --- a/tools/lib/bpf/bpf.c > +++ b/tools/lib/bpf/bpf.c > @@ -79,7 +79,6 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size) > > int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr) > { > - __u32 name_len = create_attr->name ? strlen(create_attr->name) : 0; > union bpf_attr attr; > > memset(&attr, '\0', sizeof(attr)); > @@ -89,8 +88,9 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr) > attr.value_size = create_attr->value_size; > attr.max_entries = create_attr->max_entries; > attr.map_flags = create_attr->map_flags; > - memcpy(attr.map_name, create_attr->name, > - min(name_len, BPF_OBJ_NAME_LEN - 1)); > + if (create_attr->name) > + memcpy(attr.map_name, create_attr->name, > + min(strlen(create_attr->name), BPF_OBJ_NAME_LEN - 1)); Any reason we don't simplify this to use strncpy() for all these occurrences? Thanks, Daniel
On 4/10/19 12:58 AM, Daniel Borkmann wrote: > On 04/10/2019 02:37 AM, Yonghong Song wrote: >> The issue is reported at https://github.com/libbpf/libbpf/issues/28. >> >> Basically, per C standard, for >> void *memcpy(void *dest, const void *src, size_t n) >> if "dest" or "src" is NULL, regardless of whether "n" is 0 or not, >> the result of memcpy is undefined. clang ubsan reported three such >> instances in bpf.c with the following pattern: >> memcpy(dest, 0, 0). >> >> Although in practice, no known compiler will cause issues when >> copy size is 0. Let us still fix the issue to silence ubsan >> warnings. >> >> Signed-off-by: Yonghong Song <yhs@fb.com> > > Applied, thanks. I fixed up $SUBJECT while applying to add a subsystem prefix. > >> --- >> tools/lib/bpf/bpf.c | 19 +++++++++---------- >> 1 file changed, 9 insertions(+), 10 deletions(-) >> >> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c >> index a1db869a6fda..78f2400dd2d1 100644 >> --- a/tools/lib/bpf/bpf.c >> +++ b/tools/lib/bpf/bpf.c >> @@ -79,7 +79,6 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size) >> >> int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr) >> { >> - __u32 name_len = create_attr->name ? strlen(create_attr->name) : 0; >> union bpf_attr attr; >> >> memset(&attr, '\0', sizeof(attr)); >> @@ -89,8 +88,9 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr) >> attr.value_size = create_attr->value_size; >> attr.max_entries = create_attr->max_entries; >> attr.map_flags = create_attr->map_flags; >> - memcpy(attr.map_name, create_attr->name, >> - min(name_len, BPF_OBJ_NAME_LEN - 1)); >> + if (create_attr->name) >> + memcpy(attr.map_name, create_attr->name, >> + min(strlen(create_attr->name), BPF_OBJ_NAME_LEN - 1)); > > Any reason we don't simplify this to use strncpy() for all these occurrences? No particular reason, just did not think that far :-) Yes, strncpy instead of memcpy should work here as well. > > Thanks, > Daniel >
diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c index a1db869a6fda..78f2400dd2d1 100644 --- a/tools/lib/bpf/bpf.c +++ b/tools/lib/bpf/bpf.c @@ -79,7 +79,6 @@ static inline int sys_bpf_prog_load(union bpf_attr *attr, unsigned int size) int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr) { - __u32 name_len = create_attr->name ? strlen(create_attr->name) : 0; union bpf_attr attr; memset(&attr, '\0', sizeof(attr)); @@ -89,8 +88,9 @@ int bpf_create_map_xattr(const struct bpf_create_map_attr *create_attr) attr.value_size = create_attr->value_size; attr.max_entries = create_attr->max_entries; attr.map_flags = create_attr->map_flags; - memcpy(attr.map_name, create_attr->name, - min(name_len, BPF_OBJ_NAME_LEN - 1)); + if (create_attr->name) + memcpy(attr.map_name, create_attr->name, + min(strlen(create_attr->name), BPF_OBJ_NAME_LEN - 1)); attr.numa_node = create_attr->numa_node; attr.btf_fd = create_attr->btf_fd; attr.btf_key_type_id = create_attr->btf_key_type_id; @@ -155,7 +155,6 @@ int bpf_create_map_in_map_node(enum bpf_map_type map_type, const char *name, int key_size, int inner_map_fd, int max_entries, __u32 map_flags, int node) { - __u32 name_len = name ? strlen(name) : 0; union bpf_attr attr; memset(&attr, '\0', sizeof(attr)); @@ -166,7 +165,9 @@ int bpf_create_map_in_map_node(enum bpf_map_type map_type, const char *name, attr.inner_map_fd = inner_map_fd; attr.max_entries = max_entries; attr.map_flags = map_flags; - memcpy(attr.map_name, name, min(name_len, BPF_OBJ_NAME_LEN - 1)); + if (name) + memcpy(attr.map_name, name, + min(strlen(name), BPF_OBJ_NAME_LEN - 1)); if (node >= 0) { attr.map_flags |= BPF_F_NUMA_NODE; @@ -216,7 +217,6 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, void *finfo = NULL, *linfo = NULL; union bpf_attr attr; __u32 log_level; - __u32 name_len; int fd; if (!load_attr || !log_buf != !log_buf_sz) @@ -226,8 +226,6 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, if (log_level > (4 | 2 | 1) || (log_level && !log_buf)) return -EINVAL; - name_len = load_attr->name ? strlen(load_attr->name) : 0; - memset(&attr, 0, sizeof(attr)); attr.prog_type = load_attr->prog_type; attr.expected_attach_type = load_attr->expected_attach_type; @@ -253,8 +251,9 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr, attr.line_info_rec_size = load_attr->line_info_rec_size; attr.line_info_cnt = load_attr->line_info_cnt; attr.line_info = ptr_to_u64(load_attr->line_info); - memcpy(attr.prog_name, load_attr->name, - min(name_len, BPF_OBJ_NAME_LEN - 1)); + if (load_attr->name) + memcpy(attr.prog_name, load_attr->name, + min(strlen(load_attr->name), BPF_OBJ_NAME_LEN - 1)); fd = sys_bpf_prog_load(&attr, sizeof(attr)); if (fd >= 0)
The issue is reported at https://github.com/libbpf/libbpf/issues/28. Basically, per C standard, for void *memcpy(void *dest, const void *src, size_t n) if "dest" or "src" is NULL, regardless of whether "n" is 0 or not, the result of memcpy is undefined. clang ubsan reported three such instances in bpf.c with the following pattern: memcpy(dest, 0, 0). Although in practice, no known compiler will cause issues when copy size is 0. Let us still fix the issue to silence ubsan warnings. Signed-off-by: Yonghong Song <yhs@fb.com> --- tools/lib/bpf/bpf.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)