[PATCH bpf 1/3] bpf: don't assume build-id length is always 20 bytes

[PATCH bpf 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
  * 128-bit (uuid)
  * 160-bit (sha1)
  * any length specified in ld --build-id=0xhexstring

To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
assume that build-id is somewhere in the range of 1 .. 20.
Set the remaining bytes to zero.

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index d9e2483669d0..8975d1768dcb 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -180,11 +180,15 @@ static inline int stack_map_parse_build_id(void *page_addr,
 
 		if (nhdr->n_type == BPF_BUILD_ID &&
 		    nhdr->n_namesz == sizeof("GNU") &&
-		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
+		    nhdr->n_descsz > 0 &&
+		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
+			__u32 len = min_t(__u32,
+					  BPF_BUILD_ID_SIZE, nhdr->n_descsz);
 			memcpy(build_id,
 			       note_start + note_offs +
 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
-			       BPF_BUILD_ID_SIZE);
+			       len);
+			memset(build_id + len, 0, BPF_BUILD_ID_SIZE - len);
 			return 0;
 		}
 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
-- 
2.20.1.97.g81188d93c3-goog

[PATCH bpf 2/3] bpf: zero out build_id for BPF_STACK_BUILD_ID_IP

[Stanislav Fomichev]

When returning BPF_STACK_BUILD_ID_IP from stack_map_get_build_id_offset,
make sure that build_id field is empty. Since we are using percpu
free list, there is a possibility that we might reuse some previous
bpf_stack_build_id with non-zero build_id.

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index 8975d1768dcb..f4b57c68c45f 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -315,6 +315,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 		for (i = 0; i < trace_nr; i++) {
 			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
 			id_offs[i].ip = ips[i];
+			memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
 		}
 		return;
 	}
@@ -325,6 +326,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
 			/* per entry fall back to ips */
 			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
 			id_offs[i].ip = ips[i];
+			memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
 			continue;
 		}
 		id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ips[i]
-- 
2.20.1.97.g81188d93c3-goog

[PATCH bpf 3/3] selftests/bpf: retry tests that expect build-id

[Stanislav Fomichev]

While running test_progs in a loop I found out that I'm sometimes hitting
"Didn't find expected build ID from the map" error.
Looking at stack_map_get_build_id_offset() it seems that it is racy (by
design) and can sometimes return BPF_STACK_BUILD_ID_IP (i.e. can't trylock
current->mm->mmap_sem).

Let's retry this test a single time.

Fixes: 13790d1cc72c ("bpf: add selftest for stackmap with build_id in NMI context")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/test_progs.c | 30 ++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 126fc624290d..25f0083a9b2e 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -1188,7 +1188,9 @@ static void test_stacktrace_build_id(void)
 	int i, j;
 	struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
 	int build_id_matches = 0;
+	int retry = 1;
 
+retry:
 	err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
 	if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
 		goto out;
@@ -1301,6 +1303,19 @@ static void test_stacktrace_build_id(void)
 		previous_key = key;
 	} while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
 
+	/* stack_map_get_build_id_offset() is racy and sometimes can return
+	 * BPF_STACK_BUILD_ID_IP instead of BPF_STACK_BUILD_ID_VALID;
+	 * try it one more time.
+	 */
+	if (build_id_matches < 1 && retry--) {
+		ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
+		close(pmu_fd);
+		bpf_object__close(obj);
+		printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+		       __func__);
+		goto retry;
+	}
+
 	if (CHECK(build_id_matches < 1, "build id match",
 		  "Didn't find expected build ID from the map\n"))
 		goto disable_pmu;
@@ -1341,7 +1356,9 @@ static void test_stacktrace_build_id_nmi(void)
 	int i, j;
 	struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
 	int build_id_matches = 0;
+	int retry = 1;
 
+retry:
 	err = bpf_prog_load(file, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd);
 	if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
 		return;
@@ -1436,6 +1453,19 @@ static void test_stacktrace_build_id_nmi(void)
 		previous_key = key;
 	} while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
 
+	/* stack_map_get_build_id_offset() is racy and sometimes can return
+	 * BPF_STACK_BUILD_ID_IP instead of BPF_STACK_BUILD_ID_VALID;
+	 * try it one more time.
+	 */
+	if (build_id_matches < 1 && retry--) {
+		ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
+		close(pmu_fd);
+		bpf_object__close(obj);
+		printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+		       __func__);
+		goto retry;
+	}
+
 	if (CHECK(build_id_matches < 1, "build id match",
 		  "Didn't find expected build ID from the map\n"))
 		goto disable_pmu;
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH bpf 1/3] bpf: don't assume build-id length is always 20 bytes

[Song Liu]

> On Jan 15, 2019, at 2:54 PM, Stanislav Fomichev <sdf@google.com> wrote:
> 
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>  * 128-bit (uuid)
>  * 160-bit (sha1)
>  * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> kernel/bpf/stackmap.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index d9e2483669d0..8975d1768dcb 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -180,11 +180,15 @@ static inline int stack_map_parse_build_id(void *page_addr,
> 
> 		if (nhdr->n_type == BPF_BUILD_ID &&
> 		    nhdr->n_namesz == sizeof("GNU") &&
> -		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
> +		    nhdr->n_descsz > 0 &&
> +		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
> +			__u32 len = min_t(__u32,
> +					  BPF_BUILD_ID_SIZE, nhdr->n_descsz);

Given the check above, we only need len = nhdr->n_descsz, right?

Other than this, 

Acked-by: Song Liu <songliubraving@fb.com>

Thanks for the fix!

> 			memcpy(build_id,
> 			       note_start + note_offs +
> 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
> -			       BPF_BUILD_ID_SIZE);
> +			       len);
> +			memset(build_id + len, 0, BPF_BUILD_ID_SIZE - len);
> 			return 0;
> 		}
> 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

Re: [PATCH bpf 2/3] bpf: zero out build_id for BPF_STACK_BUILD_ID_IP

[Song Liu]

> On Jan 15, 2019, at 2:54 PM, Stanislav Fomichev <sdf@google.com> wrote:
> 
> When returning BPF_STACK_BUILD_ID_IP from stack_map_get_build_id_offset,
> make sure that build_id field is empty. Since we are using percpu
> free list, there is a possibility that we might reuse some previous
> bpf_stack_build_id with non-zero build_id.
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Acked-by: Song Liu <songliubraving@fb.com>

> ---
> kernel/bpf/stackmap.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index 8975d1768dcb..f4b57c68c45f 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -315,6 +315,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> 		for (i = 0; i < trace_nr; i++) {
> 			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> 			id_offs[i].ip = ips[i];
> +			memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
> 		}
> 		return;
> 	}
> @@ -325,6 +326,7 @@ static void stack_map_get_build_id_offset(struct bpf_stack_build_id *id_offs,
> 			/* per entry fall back to ips */
> 			id_offs[i].status = BPF_STACK_BUILD_ID_IP;
> 			id_offs[i].ip = ips[i];
> +			memset(id_offs[i].build_id, 0, BPF_BUILD_ID_SIZE);
> 			continue;
> 		}
> 		id_offs[i].offset = (vma->vm_pgoff << PAGE_SHIFT) + ips[i]
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

Re: [PATCH bpf 3/3] selftests/bpf: retry tests that expect build-id

[Song Liu]

> On Jan 15, 2019, at 2:54 PM, Stanislav Fomichev <sdf@google.com> wrote:
> 
> While running test_progs in a loop I found out that I'm sometimes hitting
> "Didn't find expected build ID from the map" error.
> Looking at stack_map_get_build_id_offset() it seems that it is racy (by
> design) and can sometimes return BPF_STACK_BUILD_ID_IP (i.e. can't trylock
> current->mm->mmap_sem).
> 
> Let's retry this test a single time.
> 
> Fixes: 13790d1cc72c ("bpf: add selftest for stackmap with build_id in NMI context")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Acked-by: Song Liu <songliubraving@fb.com>


> ---
> tools/testing/selftests/bpf/test_progs.c | 30 ++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index 126fc624290d..25f0083a9b2e 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -1188,7 +1188,9 @@ static void test_stacktrace_build_id(void)
> 	int i, j;
> 	struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> 	int build_id_matches = 0;
> +	int retry = 1;
> 
> +retry:
> 	err = bpf_prog_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
> 	if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
> 		goto out;
> @@ -1301,6 +1303,19 @@ static void test_stacktrace_build_id(void)
> 		previous_key = key;
> 	} while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
> 
> +	/* stack_map_get_build_id_offset() is racy and sometimes can return
> +	 * BPF_STACK_BUILD_ID_IP instead of BPF_STACK_BUILD_ID_VALID;
> +	 * try it one more time.
> +	 */
> +	if (build_id_matches < 1 && retry--) {
> +		ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> +		close(pmu_fd);
> +		bpf_object__close(obj);
> +		printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> +		       __func__);
> +		goto retry;
> +	}
> +
> 	if (CHECK(build_id_matches < 1, "build id match",
> 		  "Didn't find expected build ID from the map\n"))
> 		goto disable_pmu;
> @@ -1341,7 +1356,9 @@ static void test_stacktrace_build_id_nmi(void)
> 	int i, j;
> 	struct bpf_stack_build_id id_offs[PERF_MAX_STACK_DEPTH];
> 	int build_id_matches = 0;
> +	int retry = 1;
> 
> +retry:
> 	err = bpf_prog_load(file, BPF_PROG_TYPE_PERF_EVENT, &obj, &prog_fd);
> 	if (CHECK(err, "prog_load", "err %d errno %d\n", err, errno))
> 		return;
> @@ -1436,6 +1453,19 @@ static void test_stacktrace_build_id_nmi(void)
> 		previous_key = key;
> 	} while (bpf_map_get_next_key(stackmap_fd, &previous_key, &key) == 0);
> 
> +	/* stack_map_get_build_id_offset() is racy and sometimes can return
> +	 * BPF_STACK_BUILD_ID_IP instead of BPF_STACK_BUILD_ID_VALID;
> +	 * try it one more time.
> +	 */
> +	if (build_id_matches < 1 && retry--) {
> +		ioctl(pmu_fd, PERF_EVENT_IOC_DISABLE);
> +		close(pmu_fd);
> +		bpf_object__close(obj);
> +		printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
> +		       __func__);
> +		goto retry;
> +	}
> +
> 	if (CHECK(build_id_matches < 1, "build id match",
> 		  "Didn't find expected build ID from the map\n"))
> 		goto disable_pmu;
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

Re: [PATCH bpf 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

On Wed, Jan 16, 2019 at 9:45 AM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Jan 15, 2019, at 2:54 PM, Stanislav Fomichev <sdf@google.com> wrote:
> >
> > Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
> >  * 128-bit (uuid)
> >  * 160-bit (sha1)
> >  * any length specified in ld --build-id=0xhexstring
> >
> > To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> > assume that build-id is somewhere in the range of 1 .. 20.
> > Set the remaining bytes to zero.
> >
> > Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > kernel/bpf/stackmap.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> > index d9e2483669d0..8975d1768dcb 100644
> > --- a/kernel/bpf/stackmap.c
> > +++ b/kernel/bpf/stackmap.c
> > @@ -180,11 +180,15 @@ static inline int stack_map_parse_build_id(void *page_addr,
> >
> >               if (nhdr->n_type == BPF_BUILD_ID &&
> >                   nhdr->n_namesz == sizeof("GNU") &&
> > -                 nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
> > +                 nhdr->n_descsz > 0 &&
> > +                 nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
> > +                     __u32 len = min_t(__u32,
> > +                                       BPF_BUILD_ID_SIZE, nhdr->n_descsz);
>
> Given the check above, we only need len = nhdr->n_descsz, right?
Ah, correct, I'll fix in v2.
I initially had without `if (nhdr->n_descsz <= BPF_BUILD_ID_SIZE)` and
clamped it here, but then decided that clamping is probably bad as
well.
>
> Other than this,
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> Thanks for the fix!
>
> >                       memcpy(build_id,
> >                              note_start + note_offs +
> >                              ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
> > -                            BPF_BUILD_ID_SIZE);
> > +                            len);
> > +                     memset(build_id + len, 0, BPF_BUILD_ID_SIZE - len);
> >                       return 0;
> >               }
> >               new_offs = note_offs + sizeof(Elf32_Nhdr) +
> > --
> > 2.20.1.97.g81188d93c3-goog
> >
>

[PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
  * 128-bit (uuid)
  * 160-bit (sha1)
  * any length specified in ld --build-id=0xhexstring

To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
assume that build-id is somewhere in the range of 1 .. 20.
Set the remaining bytes to zero.

v2:
* don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
  we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
  this 'if' condition

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index d9e2483669d0..f9df545e92f6 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -180,11 +180,14 @@ static inline int stack_map_parse_build_id(void *page_addr,
 
 		if (nhdr->n_type == BPF_BUILD_ID &&
 		    nhdr->n_namesz == sizeof("GNU") &&
-		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
+		    nhdr->n_descsz > 0 &&
+		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
 			memcpy(build_id,
 			       note_start + note_offs +
 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
-			       BPF_BUILD_ID_SIZE);
+			       nhdr->n_descsz);
+			memset(build_id + nhdr->n_descsz, 0,
+			       BPF_BUILD_ID_SIZE - nhdr->n_descsz);
 			return 0;
 		}
 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Song Liu]

> On Jan 16, 2019, at 10:11 AM, Stanislav Fomichev <sdf@google.com> wrote:
> 
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>  * 128-bit (uuid)
>  * 160-bit (sha1)
>  * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> v2:
> * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
>  we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
>  this 'if' condition
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Acked-by: Song Liu <songliubraving@fb.com>
 

> ---
> kernel/bpf/stackmap.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
> index d9e2483669d0..f9df545e92f6 100644
> --- a/kernel/bpf/stackmap.c
> +++ b/kernel/bpf/stackmap.c
> @@ -180,11 +180,14 @@ static inline int stack_map_parse_build_id(void *page_addr,
> 
> 		if (nhdr->n_type == BPF_BUILD_ID &&
> 		    nhdr->n_namesz == sizeof("GNU") &&
> -		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
> +		    nhdr->n_descsz > 0 &&
> +		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
> 			memcpy(build_id,
> 			       note_start + note_offs +
> 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
> -			       BPF_BUILD_ID_SIZE);
> +			       nhdr->n_descsz);
> +			memset(build_id + nhdr->n_descsz, 0,
> +			       BPF_BUILD_ID_SIZE - nhdr->n_descsz);
> 			return 0;
> 		}
> 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
> -- 
> 2.20.1.97.g81188d93c3-goog
> 

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Daniel Borkmann]

On 01/16/2019 07:11 PM, Stanislav Fomichev wrote:
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>   * 128-bit (uuid)
>   * 160-bit (sha1)
>   * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> v2:
> * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
>   we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
>   this 'if' condition
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Hmm, looks like rest of the v2 series didn't make it to the list.
Please double check; just in case simply resend the full v2 set so
it properly ends up in patchwork.

Thanks,
Daniel

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

On 01/16, Daniel Borkmann wrote:
> On 01/16/2019 07:11 PM, Stanislav Fomichev wrote:
> > Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
> >   * 128-bit (uuid)
> >   * 160-bit (sha1)
> >   * any length specified in ld --build-id=0xhexstring
> > 
> > To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> > assume that build-id is somewhere in the range of 1 .. 20.
> > Set the remaining bytes to zero.
> > 
> > v2:
> > * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
> >   we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
> >   this 'if' condition
> > 
> > Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> 
> Hmm, looks like rest of the v2 series didn't make it to the list.
> Please double check; just in case simply resend the full v2 set so
> it properly ends up in patchwork.
Oh, I didn't send the first two patches because I didn't change them, I'll
resend full v2 series in a moment.
> 
> Thanks,
> Daniel

[PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Stanislav Fomichev]

Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
  * 128-bit (uuid)
  * 160-bit (sha1)
  * any length specified in ld --build-id=0xhexstring

To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
assume that build-id is somewhere in the range of 1 .. 20.
Set the remaining bytes to zero.

v2:
* don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
  we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
  this 'if' condition

Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
Acked-by: Song Liu <songliubraving@fb.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 kernel/bpf/stackmap.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/stackmap.c b/kernel/bpf/stackmap.c
index d9e2483669d0..f9df545e92f6 100644
--- a/kernel/bpf/stackmap.c
+++ b/kernel/bpf/stackmap.c
@@ -180,11 +180,14 @@ static inline int stack_map_parse_build_id(void *page_addr,
 
 		if (nhdr->n_type == BPF_BUILD_ID &&
 		    nhdr->n_namesz == sizeof("GNU") &&
-		    nhdr->n_descsz == BPF_BUILD_ID_SIZE) {
+		    nhdr->n_descsz > 0 &&
+		    nhdr->n_descsz <= BPF_BUILD_ID_SIZE) {
 			memcpy(build_id,
 			       note_start + note_offs +
 			       ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_Nhdr),
-			       BPF_BUILD_ID_SIZE);
+			       nhdr->n_descsz);
+			memset(build_id + nhdr->n_descsz, 0,
+			       BPF_BUILD_ID_SIZE - nhdr->n_descsz);
 			return 0;
 		}
 		new_offs = note_offs + sizeof(Elf32_Nhdr) +
-- 
2.20.1.97.g81188d93c3-goog

Re: [PATCH bpf v2 1/3] bpf: don't assume build-id length is always 20 bytes

[Daniel Borkmann]

On 01/16/2019 11:03 PM, Stanislav Fomichev wrote:
> Build-id length is not fixed to 20, it can be (`man ld` /--build-id):
>   * 128-bit (uuid)
>   * 160-bit (sha1)
>   * any length specified in ld --build-id=0xhexstring
> 
> To fix the issue of missing BPF_STACK_BUILD_ID_VALID for shorter build-ids,
> assume that build-id is somewhere in the range of 1 .. 20.
> Set the remaining bytes to zero.
> 
> v2:
> * don't introduce new "len = min(BPF_BUILD_ID_SIZE, nhdr->n_descsz)",
>   we already know that nhdr->n_descsz <= BPF_BUILD_ID_SIZE if we enter
>   this 'if' condition
> 
> Fixes: 615755a77b24 ("bpf: extend stackmap to save binary_build_id+offset instead of address")
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Series applied, thanks!