Re: [RFC PATCH bpf-next] bpf: Fix an error in verifying a field in a union

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 23, 2023 at 7:42 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> 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.

It seems we can't check the flag, because it clears the flag when it
enters btf_struct_walk()[1].
We only set it when we find a nested union, but we don't set this flag
when the btf_type itself is a union. So that can't apply to `union
bpf_attr`.

[1]. https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/tree/kernel/bpf/btf.c#n6140

-- 
Regards
Yafang





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux