On Tue, Jul 11, 2023 at 10:56 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Sat, Jul 8, 2023 at 7:59 PM 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.
> >
> > Fixes: 7e3617a72df3 ("bpf: Add array support to btf_struct_access")
> > Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> > ---
> > 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 fae6fc24a845..a542760c807a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -6368,7 +6368,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 && !(*flag & PTR_UNTRUSTED)) {
>
> The selftest for this condition is missing.
Will add it.
--
Regards
Yafang
