linux-kernel - Re: [PATCH bpf-next v7 1/3] bpf: Allow trusted pointers to be passed to KF_TRUSTED

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 linux-cve-announce PHC
Open Source and information security mailing list archives

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

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

Message-ID: <20221119041337.eejp2dfe6w5xqplo@macbook-pro-5.dhcp.thefacebook.com>
Date:   Fri, 18 Nov 2022 20:13:37 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     David Vernet <void@...ifault.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, andrii@...nel.org,
        daniel@...earbox.net, martin.lau@...ux.dev, memxor@...il.com,
        yhs@...com, song@...nel.org, sdf@...gle.com,
        john.fastabend@...il.com, kpsingh@...nel.org, jolsa@...nel.org,
        haoluo@...gle.com, tj@...nel.org, kernel-team@...com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v7 1/3] bpf: Allow trusted pointers to be passed
 to KF_TRUSTED_ARGS kfuncs

On Fri, Nov 18, 2022 at 03:44:42PM -0600, David Vernet wrote:
> > > if it's a release arg it should always have a refcount on it.
> > > PTR_UNTRUSTED | PTR_TRUSTED would also make no sense. MEM_FIXED_SIZE
> > > though seems fine? In general, I thought it was prudent for us to take
> > > the most conservative possible approach here, which is that PTR_TRUSTED
> > > only applies when no other modifiers are present, and it applies for all
> > > obj_ptr types (other than PTR_TO_CTX which does its own thing).
> > 
> > Probably worth refining when PTR_TRUSTED is cleared.
> > For example adding PTR_UNTRUSTED should definitely clear it.
> 
> That makes sense for PTR_UNTRUSTED, what about the other type modifiers
> like PTR_MAYBE_NULL? We set and unset if a ptr is NULL throughout a
> function, so we'd have to record if it was previously trusted in order
> to properly re-OR after a NULL check.

PTR_MAYBE_NULL is another bit and I don't think it conflicts with PTR_TRUSTED.
PTR_TO_BTF_ID | PTR_TRUSTED is a valid pointer.
PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL is a valid pointer or NULL.

PTR_TO_BTF_ID | PTR_MAYBE_NULL is a legacy "valid pointer" or NULL.
That legacy pointer cannot be passed to KF_TRUSTED_ARGS kfuncs.

KF_TRUSTED_ARGS kfuncs should not accept PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL.
It's a job of the prog to do != NULL check.
Otherwise all such != NULL checks would need to move inside kfuncs which is not good.

> > MEM_ALLOC flag is probably equivalent to PTR_TRUSTED.
> > Maybe the bit:
> > regs[BPF_REG_0].type = PTR_TO_BTF_ID | MEM_ALLOC;
> > should set PTR_TRUSTED as well?
> 
> We could, but that changes the meaning of PTR_TRUSTED and IMO makes it
> harder to reason about. Before it was just "the kernel passed this arg
> to the program and promises the program that it was trusted when it was
> first passed". Now it's that plus it could mean that it points to an
> allocated object from bpf_obj_new()". IMO we should keep all of these
> modifiers separate so that the presence of a modifier has a well-defined
> meaning that we can interpret in each context as needed.  In this case,
> we can make trust opt-in, so a KF_TRUSTED_ARGS BTF pointer either of the
> following:
> 
> 1. reg->ref_obj_id > 0
> 2. Either one of PTR_TRUSTED | MEM_ALLOC type modifiers are set, and no
>    others.

I don't think MEM_ALLOC conflicts with PTR_TRUSTED.
MEM_ALLOC flags means that it came from bpf_obj_new() and that's what
bpf_spin_lock and bpf_obj_drop() want to see.

Adding PTR_TRUSTED to MEM_ALLOC looks necessary to me.
It doesn't have to be done right now, but eventually feels right.

I've been thinking whether reg->ref_obj_id > 0 condition should be converted
to PTR_TRUSTED too...
On one side it will simplify the check for KF_TRUSTED_ARGS.
The only thing check_kfunc_args() would need to do is:
is_kfunc_trusted_args(meta)
&& type_flag(reg->type) & PTR_TRUSTED
&& !(type_flag(reg->type) & PTR_MAYBE_NULL)

On the other side fixing all places where we set ref_obj_id
and adding |= PTR_TRUSTED may be too cumbersome ?

Right now we're saying PTR_TO_CTX is implicitly trusted, but we can OR
PTR_TO_CTX with PTR_TRUSTED to make it explicit and truly generalize the check.

> Agreed that after the rebase this would no longer be correct. I think we
> should make it opt-in, though. PTR_TRUSTED | MEM_ALLOC is fine.
> PTR_TRUSTED | MEM_ALLOC | PTR_MAYBE_NULL would not be.

to pass into KF_TRUSTED_ARGS kfunc? Agree.
I guess we can tighten the check a bit:
is_kfunc_trusted_args(meta)
&& type_flag(reg->type) & PTR_TRUSTED
&& !(type_flag(reg->type) & ~(PTR_TRUSTED | MEM_ALLOC))

In english: the pointer should have PTR_TRUSTED flag and
no other flags other than PTR_TRUSTED and MEM_ALLOC should be set.