netdev - Re: [PATCH v10 bpf-next 08/14] bpf: Add btf_struct_ids

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sun, 9 Aug 2020 16:47:06 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Jiri Olsa <jolsa@...nel.org>, Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andriin@...com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Wenbo Zhang <ethercflow@...il.com>,
        KP Singh <kpsingh@...omium.org>,
        Brendan Gregg <bgregg@...flix.com>,
        Florent Revest <revest@...omium.org>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v10 bpf-next 08/14] bpf: Add btf_struct_ids_match function

On Fri, Aug 07, 2020 at 01:04:26PM -0700, Andrii Nakryiko wrote:

SNIP

> > +                               }
> >                         }
> >                 } else if (!fn->check_btf_id(reg->btf_id, arg)) {
> 
> Put this on a wishlist for now. I don't think we should expect
> fb->check_btf_id() to do btf_struct_ids_match() internally, so to
> support this, we'd have to call fb->check_btf_id() inside the loop
> while doing WALK_STRUCT struct. But let's not change all this in this
> patch set, it's involved enough already.
> 
> >                         verbose(env, "Helper does not support %s in R%d\n",
> > @@ -3977,7 +3982,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
> >
> >                         return -EACCES;
> >                 }
> > -               if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) {
> > +               if (!ids_match &&
> > +                   (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) {
> 
> Isn't this still wrong? if ids_match, but reg->var_off is non-zero,
> that's still bad, right?
> ids_match just "mitigates" reg->off check, so should be something like this:
> 
> if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) ||
> reg->var_off.value)
>  ... then bad ...

damn you're right, those are separated things,
I mixed it up, I'll send new version

thanks,
jirka