On Wed, Jun 21, 2023 at 5:00 AM Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
>
> We are utilizing BPF LSM to monitor BPF operations within our container
> environment. When we add support for raw_tracepoint, it hits below
> error.
>
> ; (const void *)attr->raw_tracepoint.name);
> 27: (79) r3 = *(u64 *)(r2 +0)
> access beyond the end of member map_type (mend:4) in struct (anon) with off 0 size 8
>
> It can be reproduced with below BPF prog.
>
> SEC("lsm/bpf")
> int BPF_PROG(bpf_audit, int cmd, union bpf_attr *attr, unsigned int size)
> {
> switch (cmd) {
> case BPF_RAW_TRACEPOINT_OPEN:
> bpf_printk("raw_tracepoint is %s", attr->raw_tracepoint.name);
> break;
> default:
> break;
> }
> return 0;
> }
>
> The reason is that when accessing a field in a union, such as bpf_attr, if
> the field is located within a nested struct that is not the first member of
> the union, it can result in incorrect field verification.
>
> union bpf_attr {
> struct {
> __u32 map_type; <<<< Actually it will find that field.
> __u32 key_size;
> __u32 value_size;
> ...
> };
> ...
> struct {
> __u64 name; <<<< We want to verify this field.
> __u32 prog_fd;
> } raw_tracepoint;
> };
>
> Considering the potential deep nesting levels, finding a perfect solution
> to address this issue has proven challenging. Therefore, I propose a
> solution where we simply skip the verification process if the field in
> question is located within a union.
>
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> ---
> kernel/bpf/btf.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index bd2cac057928..79ee4506bba4 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -6129,7 +6129,7 @@ enum bpf_struct_walk_result {
> static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> const struct btf_type *t, int off, int size,
> u32 *next_btf_id, enum bpf_type_flag *flag,
> - const char **field_name)
> + const char **field_name, bool *in_union)
> {
> u32 i, moff, mtrue_end, msize = 0, total_nelems = 0;
> const struct btf_type *mtype, *elem_type = NULL;
> @@ -6188,6 +6188,8 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> return -EACCES;
> }
>
> + if (BTF_INFO_KIND(t->info) == BTF_KIND_UNION && !in_union)
> + *in_union = true;
> for_each_member(i, t, member) {
> /* offset of the field in bytes */
> moff = __btf_member_bit_offset(t, member) / 8;
> @@ -6372,7 +6374,7 @@ static int btf_struct_walk(struct bpf_verifier_log *log, const struct btf *btf,
> * that also allows using an array of int as a scratch
> * space. e.g. skb->cb[].
> */
> - if (off + size > mtrue_end) {
> + if (off + size > mtrue_end && !in_union) {
Just allow it for (flag & PTR_UNTRUSTED).
We set it when we start walking BTF_KIND_UNION.
No need for extra bool.
