diff mbox series

[v2,bpf-next] libbpf: btf_dump_type_data_check_overflow needs to consider BTF_MEMBER_BITFIELD_SIZE

Message ID 20230428013638.1581263-1-martin.lau@linux.dev (mailing list archive)
State Accepted
Commit c39028b333f3a3a765c5c0b9726b8e38aedf0ba1
Delegated to: BPF
Headers show
Series [v2,bpf-next] libbpf: btf_dump_type_data_check_overflow needs to consider BTF_MEMBER_BITFIELD_SIZE | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 8 this patch: 8
netdev/cc_maintainers warning 7 maintainers not CCed: yhs@fb.com kpsingh@kernel.org john.fastabend@gmail.com song@kernel.org sdf@google.com jolsa@kernel.org haoluo@google.com
netdev/build_clang success Errors and warnings before: 8 this patch: 8
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8 this patch: 8
netdev/checkpatch warning WARNING: Possible repeated word: 'unexpected' WARNING: line length of 85 exceeds 80 columns WARNING: line length of 86 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-3 success Logs for build for aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-4 success Logs for build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for build for x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-7 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-8 success Logs for test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for test_maps on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-10 success Logs for test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for test_maps on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-13 success Logs for test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for test_progs on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-15 success Logs for test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for test_progs on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-18 success Logs for test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for test_progs_no_alu32 on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-20 success Logs for test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for test_progs_no_alu32 on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-23 success Logs for test_progs_no_alu32_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for test_progs_no_alu32_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-25 success Logs for test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for test_progs_no_alu32_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-27 success Logs for test_progs_parallel on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for test_progs_parallel on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-29 success Logs for test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for test_progs_parallel on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-31 success Logs for test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for test_verifier on aarch64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-33 success Logs for test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-35 success Logs for test_verifier on x86_64 with llvm-16
bpf/vmtest-bpf-next-VM_Test-36 success Logs for veristat

Commit Message

Martin KaFai Lau April 28, 2023, 1:36 a.m. UTC 
From: Martin KaFai Lau <martin.lau@kernel.org>

The btf_dump/struct_data selftest is failing with:
test_btf_dump_struct_data:FAIL:unexpected return value dumping fs_context unexpected unexpected return value dumping fs_context: actual -7 != expected 264

The reason is in btf_dump_type_data_check_overflow(). It does not use
BTF_MEMBER_BITFIELD_SIZE from the struct's member (btf_member). Instead,
it is using the enum size which is 4. It had been working till the recent
commit 4e04143c869c ("fs_context: drop the unused lsm_flags member")
removed an integer member which also removed the 4 bytes padding at the end
of the fs_context. Missing this 4 bytes padding exposed this bug.
In particular, when btf_dump_type_data_check_overflow() reaches
the member 'phase', -E2BIG is returned.

The fix is to pass bit_sz to btf_dump_type_data_check_overflow().
In btf_dump_type_data_check_overflow(), it does a different size
check when bit_sz is not zero.

The current fs_context:

[3600] ENUM 'fs_context_purpose' encoding=UNSIGNED size=4 vlen=3
	'FS_CONTEXT_FOR_MOUNT' val=0
	'FS_CONTEXT_FOR_SUBMOUNT' val=1
	'FS_CONTEXT_FOR_RECONFIGURE' val=2
[3601] ENUM 'fs_context_phase' encoding=UNSIGNED size=4 vlen=7
	'FS_CONTEXT_CREATE_PARAMS' val=0
	'FS_CONTEXT_CREATING' val=1
	'FS_CONTEXT_AWAITING_MOUNT' val=2
	'FS_CONTEXT_AWAITING_RECONF' val=3
	'FS_CONTEXT_RECONF_PARAMS' val=4
	'FS_CONTEXT_RECONFIGURING' val=5
	'FS_CONTEXT_FAILED' val=6
[3602] STRUCT 'fs_context' size=264 vlen=21
	'ops' type_id=3603 bits_offset=0
	'uapi_mutex' type_id=235 bits_offset=64
	'fs_type' type_id=872 bits_offset=1216
	'fs_private' type_id=21 bits_offset=1280
	'sget_key' type_id=21 bits_offset=1344
	'root' type_id=781 bits_offset=1408
	'user_ns' type_id=251 bits_offset=1472
	'net_ns' type_id=984 bits_offset=1536
	'cred' type_id=1785 bits_offset=1600
	'log' type_id=3621 bits_offset=1664
	'source' type_id=42 bits_offset=1792
	'security' type_id=21 bits_offset=1856
	's_fs_info' type_id=21 bits_offset=1920
	'sb_flags' type_id=20 bits_offset=1984
	'sb_flags_mask' type_id=20 bits_offset=2016
	's_iflags' type_id=20 bits_offset=2048
	'purpose' type_id=3600 bits_offset=2080 bitfield_size=8
	'phase' type_id=3601 bits_offset=2088 bitfield_size=8
	'need_free' type_id=67 bits_offset=2096 bitfield_size=1
	'global' type_id=67 bits_offset=2097 bitfield_size=1
	'oldapi' type_id=67 bits_offset=2098 bitfield_size=1

Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
v2: Return nr_bytes instead 0 for success case

 tools/lib/bpf/btf_dump.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Yonghong Song April 28, 2023, 2:26 a.m. UTC | #1 
On 4/27/23 6:36 PM, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> The btf_dump/struct_data selftest is failing with:
> test_btf_dump_struct_data:FAIL:unexpected return value dumping fs_context unexpected unexpected return value dumping fs_context: actual -7 != expected 264
> 
> The reason is in btf_dump_type_data_check_overflow(). It does not use
> BTF_MEMBER_BITFIELD_SIZE from the struct's member (btf_member). Instead,
> it is using the enum size which is 4. It had been working till the recent
> commit 4e04143c869c ("fs_context: drop the unused lsm_flags member")
> removed an integer member which also removed the 4 bytes padding at the end
> of the fs_context. Missing this 4 bytes padding exposed this bug.
> In particular, when btf_dump_type_data_check_overflow() reaches
> the member 'phase', -E2BIG is returned.
> 
> The fix is to pass bit_sz to btf_dump_type_data_check_overflow().
> In btf_dump_type_data_check_overflow(), it does a different size
> check when bit_sz is not zero.
> 
> The current fs_context:
> 
> [3600] ENUM 'fs_context_purpose' encoding=UNSIGNED size=4 vlen=3
> 	'FS_CONTEXT_FOR_MOUNT' val=0
> 	'FS_CONTEXT_FOR_SUBMOUNT' val=1
> 	'FS_CONTEXT_FOR_RECONFIGURE' val=2
> [3601] ENUM 'fs_context_phase' encoding=UNSIGNED size=4 vlen=7
> 	'FS_CONTEXT_CREATE_PARAMS' val=0
> 	'FS_CONTEXT_CREATING' val=1
> 	'FS_CONTEXT_AWAITING_MOUNT' val=2
> 	'FS_CONTEXT_AWAITING_RECONF' val=3
> 	'FS_CONTEXT_RECONF_PARAMS' val=4
> 	'FS_CONTEXT_RECONFIGURING' val=5
> 	'FS_CONTEXT_FAILED' val=6
> [3602] STRUCT 'fs_context' size=264 vlen=21
> 	'ops' type_id=3603 bits_offset=0
> 	'uapi_mutex' type_id=235 bits_offset=64
> 	'fs_type' type_id=872 bits_offset=1216
> 	'fs_private' type_id=21 bits_offset=1280
> 	'sget_key' type_id=21 bits_offset=1344
> 	'root' type_id=781 bits_offset=1408
> 	'user_ns' type_id=251 bits_offset=1472
> 	'net_ns' type_id=984 bits_offset=1536
> 	'cred' type_id=1785 bits_offset=1600
> 	'log' type_id=3621 bits_offset=1664
> 	'source' type_id=42 bits_offset=1792
> 	'security' type_id=21 bits_offset=1856
> 	's_fs_info' type_id=21 bits_offset=1920
> 	'sb_flags' type_id=20 bits_offset=1984
> 	'sb_flags_mask' type_id=20 bits_offset=2016
> 	's_iflags' type_id=20 bits_offset=2048
> 	'purpose' type_id=3600 bits_offset=2080 bitfield_size=8
> 	'phase' type_id=3601 bits_offset=2088 bitfield_size=8
> 	'need_free' type_id=67 bits_offset=2096 bitfield_size=1
> 	'global' type_id=67 bits_offset=2097 bitfield_size=1
> 	'oldapi' type_id=67 bits_offset=2098 bitfield_size=1
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>

Acked-by: Yonghong Song <yhs@fb.com>
Martin KaFai Lau April 28, 2023, 2:58 a.m. UTC | #2 
On 4/27/23 6:36 PM, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> The btf_dump/struct_data selftest is failing with:
> test_btf_dump_struct_data:FAIL:unexpected return value dumping fs_context unexpected unexpected return value dumping fs_context: actual -7 != expected 264
> 
> The reason is in btf_dump_type_data_check_overflow(). It does not use
> BTF_MEMBER_BITFIELD_SIZE from the struct's member (btf_member). Instead,
> it is using the enum size which is 4. It had been working till the recent
> commit 4e04143c869c ("fs_context: drop the unused lsm_flags member")
> removed an integer member which also removed the 4 bytes padding at the end
> of the fs_context. Missing this 4 bytes padding exposed this bug.
> In particular, when btf_dump_type_data_check_overflow() reaches
> the member 'phase', -E2BIG is returned.
> 
> The fix is to pass bit_sz to btf_dump_type_data_check_overflow().
> In btf_dump_type_data_check_overflow(), it does a different size
> check when bit_sz is not zero.
> 
> The current fs_context:
> 
> [3600] ENUM 'fs_context_purpose' encoding=UNSIGNED size=4 vlen=3
> 	'FS_CONTEXT_FOR_MOUNT' val=0
> 	'FS_CONTEXT_FOR_SUBMOUNT' val=1
> 	'FS_CONTEXT_FOR_RECONFIGURE' val=2
> [3601] ENUM 'fs_context_phase' encoding=UNSIGNED size=4 vlen=7
> 	'FS_CONTEXT_CREATE_PARAMS' val=0
> 	'FS_CONTEXT_CREATING' val=1
> 	'FS_CONTEXT_AWAITING_MOUNT' val=2
> 	'FS_CONTEXT_AWAITING_RECONF' val=3
> 	'FS_CONTEXT_RECONF_PARAMS' val=4
> 	'FS_CONTEXT_RECONFIGURING' val=5
> 	'FS_CONTEXT_FAILED' val=6
> [3602] STRUCT 'fs_context' size=264 vlen=21
> 	'ops' type_id=3603 bits_offset=0
> 	'uapi_mutex' type_id=235 bits_offset=64
> 	'fs_type' type_id=872 bits_offset=1216
> 	'fs_private' type_id=21 bits_offset=1280
> 	'sget_key' type_id=21 bits_offset=1344
> 	'root' type_id=781 bits_offset=1408
> 	'user_ns' type_id=251 bits_offset=1472
> 	'net_ns' type_id=984 bits_offset=1536
> 	'cred' type_id=1785 bits_offset=1600
> 	'log' type_id=3621 bits_offset=1664
> 	'source' type_id=42 bits_offset=1792
> 	'security' type_id=21 bits_offset=1856
> 	's_fs_info' type_id=21 bits_offset=1920
> 	'sb_flags' type_id=20 bits_offset=1984
> 	'sb_flags_mask' type_id=20 bits_offset=2016
> 	's_iflags' type_id=20 bits_offset=2048
> 	'purpose' type_id=3600 bits_offset=2080 bitfield_size=8
> 	'phase' type_id=3601 bits_offset=2088 bitfield_size=8
> 	'need_free' type_id=67 bits_offset=2096 bitfield_size=1
> 	'global' type_id=67 bits_offset=2097 bitfield_size=1
> 	'oldapi' type_id=67 bits_offset=2098 bitfield_size=1
> 
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
Missed the fixes tag.

Fixes: 920d16af9b42 ("libbpf: BTF dumper support for typed data")
patchwork-bot+netdevbpf@kernel.org May 1, 2023, 1:50 p.m. UTC | #3 
Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Thu, 27 Apr 2023 18:36:38 -0700 you wrote:
> From: Martin KaFai Lau <martin.lau@kernel.org>
> 
> The btf_dump/struct_data selftest is failing with:
> test_btf_dump_struct_data:FAIL:unexpected return value dumping fs_context unexpected unexpected return value dumping fs_context: actual -7 != expected 264
> 
> The reason is in btf_dump_type_data_check_overflow(). It does not use
> BTF_MEMBER_BITFIELD_SIZE from the struct's member (btf_member). Instead,
> it is using the enum size which is 4. It had been working till the recent
> commit 4e04143c869c ("fs_context: drop the unused lsm_flags member")
> removed an integer member which also removed the 4 bytes padding at the end
> of the fs_context. Missing this 4 bytes padding exposed this bug.
> In particular, when btf_dump_type_data_check_overflow() reaches
> the member 'phase', -E2BIG is returned.
> 
> [...]

Here is the summary with links:
  - [v2,bpf-next] libbpf: btf_dump_type_data_check_overflow needs to consider BTF_MEMBER_BITFIELD_SIZE
    https://git.kernel.org/bpf/bpf-next/c/c39028b333f3

You are awesome, thank you!
diff mbox series

Patch

diff --git a/tools/lib/bpf/btf_dump.c b/tools/lib/bpf/btf_dump.c
index 580985ee5545..4d9f30bf7f01 100644
--- a/tools/lib/bpf/btf_dump.c
+++ b/tools/lib/bpf/btf_dump.c
@@ -2250,9 +2250,25 @@  static int btf_dump_type_data_check_overflow(struct btf_dump *d,
 					     const struct btf_type *t,
 					     __u32 id,
 					     const void *data,
-					     __u8 bits_offset)
+					     __u8 bits_offset,
+					     __u8 bit_sz)
 {
-	__s64 size = btf__resolve_size(d->btf, id);
+	__s64 size;
+
+	if (bit_sz) {
+		/* bits_offset is at most 7. bit_sz is at most 128. */
+		__u8 nr_bytes = (bits_offset + bit_sz + 7) / 8;
+
+		/* When bit_sz is non zero, it is called from
+		 * btf_dump_struct_data() where it only cares about
+		 * negative error value.
+		 * Return nr_bytes in success case to make it
+		 * consistent as the regular integer case below.
+		 */
+		return data + nr_bytes > d->typed_dump->data_end ? -E2BIG : nr_bytes;
+	}
+
+	size = btf__resolve_size(d->btf, id);
 
 	if (size < 0 || size >= INT_MAX) {
 		pr_warn("unexpected size [%zu] for id [%u]\n",
@@ -2407,7 +2423,7 @@  static int btf_dump_dump_type_data(struct btf_dump *d,
 {
 	int size, err = 0;
 
-	size = btf_dump_type_data_check_overflow(d, t, id, data, bits_offset);
+	size = btf_dump_type_data_check_overflow(d, t, id, data, bits_offset, bit_sz);
 	if (size < 0)
 		return size;
 	err = btf_dump_type_data_check_zero(d, t, id, data, bits_offset, bit_sz);