| Message ID | 20200228020212.16183-1-komachi.yoshiki@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf] bpf: btf: Fix BTF verification of the enum size in struct/union | expand |
On Thu, Feb 27, 2020 at 6:07 PM Yoshiki Komachi <komachi.yoshiki@gmail.com> wrote: > > btf_enum_check_member() checked if the size of "enum" as a struct > member exceeded struct_size or not. Then, the function definitely > compared it with the size of "int" now. Therefore, errors could occur > when the size of the "enum" type was changed. > > Although the size of "enum" is 4-byte by default, users can change it > as needed (e.g., the size of the following test variable is not 4-byte > but 1-byte). It can be used as a struct member as below: > > enum test : char { you can't specify that in pure C, but this will work for C: struct { enum { X, Y, Z } __attribute__((packed)) e; } tmp; Please add such a selftest, as part of fixing this bug. Thanks! Otherwise logic looks good. > X, > Y, > Z, > }; > > struct { > char a; > enum test b; > char c; > } tmp; > > With the setup above, when I tried to load BTF, the error was given > as below: > > ------------------------------------------------------------------ > > [58] STRUCT (anon) size=3 vlen=3 > a type_id=55 bits_offset=0 > b type_id=59 bits_offset=8 > c type_id=55 bits_offset=16 > [59] ENUM test size=1 vlen=3 > X val=0 > Y val=1 > Z val=2 > > [58] STRUCT (anon) size=3 vlen=3 > b type_id=59 bits_offset=8 Member exceeds struct_size > > libbpf: Error loading .BTF into kernel: -22. > > ------------------------------------------------------------------ > > The related issue was previously fixed by the commit 9eea98497951 ("bpf: > fix BTF verification of enums"). On the other hand, this patch fixes > my explained issue by using the correct size of "enum" declared in > BPF programs. > > Fixes: 179cde8cef7e ("bpf: btf: Check members of struct/union") > Signed-off-by: Yoshiki Komachi <komachi.yoshiki@gmail.com> > --- > kernel/bpf/btf.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 7871400..32ab922 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -2418,7 +2418,7 @@ static int btf_enum_check_member(struct btf_verifier_env *env, > > struct_size = struct_type->size; > bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off); > - if (struct_size - bytes_offset < sizeof(int)) { > + if (struct_size - bytes_offset < member_type->size) { > btf_verifier_log_member(env, struct_type, member, > "Member exceeds struct_size"); > return -EINVAL; > -- > 1.8.3.1 >
2020年2月29日(土) 9:23 Andrii Nakryiko <andrii.nakryiko@gmail.com>: > > On Thu, Feb 27, 2020 at 6:07 PM Yoshiki Komachi > <komachi.yoshiki@gmail.com> wrote: > > > > btf_enum_check_member() checked if the size of "enum" as a struct > > member exceeded struct_size or not. Then, the function definitely > > compared it with the size of "int" now. Therefore, errors could occur > > when the size of the "enum" type was changed. > > > > Although the size of "enum" is 4-byte by default, users can change it > > as needed (e.g., the size of the following test variable is not 4-byte > > but 1-byte). It can be used as a struct member as below: > > > > enum test : char { > > you can't specify that in pure C, but this will work for C: > > struct { > enum { X, Y, Z } __attribute__((packed)) e; > } tmp; > > Please add such a selftest, as part of fixing this bug. Thanks! > > Otherwise logic looks good. Thank you for kind comments! I will add a selftest program, and submit the next version later. Best regards, > > X, > > Y, > > Z, > > }; > > > > struct { > > char a; > > enum test b; > > char c; > > } tmp; > > > > With the setup above, when I tried to load BTF, the error was given > > as below: > > > > ------------------------------------------------------------------ > > > > [58] STRUCT (anon) size=3 vlen=3 > > a type_id=55 bits_offset=0 > > b type_id=59 bits_offset=8 > > c type_id=55 bits_offset=16 > > [59] ENUM test size=1 vlen=3 > > X val=0 > > Y val=1 > > Z val=2 > > > > [58] STRUCT (anon) size=3 vlen=3 > > b type_id=59 bits_offset=8 Member exceeds struct_size > > > > libbpf: Error loading .BTF into kernel: -22. > > > > ------------------------------------------------------------------ > > > > The related issue was previously fixed by the commit 9eea98497951 ("bpf: > > fix BTF verification of enums"). On the other hand, this patch fixes > > my explained issue by using the correct size of "enum" declared in > > BPF programs. > > > > Fixes: 179cde8cef7e ("bpf: btf: Check members of struct/union") > > Signed-off-by: Yoshiki Komachi <komachi.yoshiki@gmail.com> > > --- > > kernel/bpf/btf.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 7871400..32ab922 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -2418,7 +2418,7 @@ static int btf_enum_check_member(struct btf_verifier_env *env, > > > > struct_size = struct_type->size; > > bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off); > > - if (struct_size - bytes_offset < sizeof(int)) { > > + if (struct_size - bytes_offset < member_type->size) { > > btf_verifier_log_member(env, struct_type, member, > > "Member exceeds struct_size"); > > return -EINVAL; > > -- > > 1.8.3.1 > >
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7871400..32ab922 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -2418,7 +2418,7 @@ static int btf_enum_check_member(struct btf_verifier_env *env, struct_size = struct_type->size; bytes_offset = BITS_ROUNDDOWN_BYTES(struct_bits_off); - if (struct_size - bytes_offset < sizeof(int)) { + if (struct_size - bytes_offset < member_type->size) { btf_verifier_log_member(env, struct_type, member, "Member exceeds struct_size"); return -EINVAL;
btf_enum_check_member() checked if the size of "enum" as a struct member exceeded struct_size or not. Then, the function definitely compared it with the size of "int" now. Therefore, errors could occur when the size of the "enum" type was changed. Although the size of "enum" is 4-byte by default, users can change it as needed (e.g., the size of the following test variable is not 4-byte but 1-byte). It can be used as a struct member as below: enum test : char { X, Y, Z, }; struct { char a; enum test b; char c; } tmp; With the setup above, when I tried to load BTF, the error was given as below: ------------------------------------------------------------------ [58] STRUCT (anon) size=3 vlen=3 a type_id=55 bits_offset=0 b type_id=59 bits_offset=8 c type_id=55 bits_offset=16 [59] ENUM test size=1 vlen=3 X val=0 Y val=1 Z val=2 [58] STRUCT (anon) size=3 vlen=3 b type_id=59 bits_offset=8 Member exceeds struct_size libbpf: Error loading .BTF into kernel: -22. ------------------------------------------------------------------ The related issue was previously fixed by the commit 9eea98497951 ("bpf: fix BTF verification of enums"). On the other hand, this patch fixes my explained issue by using the correct size of "enum" declared in BPF programs. Fixes: 179cde8cef7e ("bpf: btf: Check members of struct/union") Signed-off-by: Yoshiki Komachi <komachi.yoshiki@gmail.com> --- kernel/bpf/btf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)