* [PATCH bpf-next] [tools/bpf] fix a few ubsan warning
@ 2019-04-10 0:37 Yonghong Song
2019-04-10 7:58 ` Daniel Borkmann
0 siblings, 1 reply; 3+ messages in thread
From: Yonghong Song @ 2019-04-10 0:37 UTC (permalink / raw)
To: bpf, netdev
Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
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(-)
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)
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] [tools/bpf] fix a few ubsan warning
2019-04-10 0:37 [PATCH bpf-next] [tools/bpf] fix a few ubsan warning Yonghong Song
@ 2019-04-10 7:58 ` Daniel Borkmann
2019-04-10 16:24 ` Yonghong Song
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Borkmann @ 2019-04-10 7:58 UTC (permalink / raw)
To: Yonghong Song, bpf, netdev; +Cc: Alexei Starovoitov, kernel-team
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH bpf-next] [tools/bpf] fix a few ubsan warning
2019-04-10 7:58 ` Daniel Borkmann
@ 2019-04-10 16:24 ` Yonghong Song
0 siblings, 0 replies; 3+ messages in thread
From: Yonghong Song @ 2019-04-10 16:24 UTC (permalink / raw)
To: Daniel Borkmann, bpf, netdev; +Cc: Alexei Starovoitov, Kernel Team
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-04-10 16:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-10 0:37 [PATCH bpf-next] [tools/bpf] fix a few ubsan warning Yonghong Song
2019-04-10 7:58 ` Daniel Borkmann
2019-04-10 16:24 ` Yonghong Song
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.