netdev - Re: [PATCHv4 bpf-next 07/13] bpf: Add reference tracking to verifier

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]

Date:   Tue, 8 Jan 2019 21:27:44 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Joe Stringer <joe@...d.net.nz>
Cc:     daniel@...earbox.net, netdev@...r.kernel.org, ast@...nel.org,
        john.fastabend@...il.com
Subject: Re: [PATCHv4 bpf-next 07/13] bpf: Add reference tracking to verifier

On Tue, Oct 02, 2018 at 01:35:35PM -0700, Joe Stringer wrote:
> Allow helper functions to acquire a reference and return it into a
> register. Specific pointer types such as the PTR_TO_SOCKET will
> implicitly represent such a reference. The verifier must ensure that
> these references are released exactly once in each path through the
> program.
> 
> To achieve this, this commit assigns an id to the pointer and tracks it
> in the 'bpf_func_state', then when the function or program exits,
> verifies that all of the acquired references have been freed. When the
> pointer is passed to a function that frees the reference, it is removed
> from the 'bpf_func_state` and all existing copies of the pointer in
> registers are marked invalid.
> 
> Signed-off-by: Joe Stringer <joe@...d.net.nz>
> Acked-by: Alexei Starovoitov <ast@...nel.org>
...
> +static void release_reg_references(struct bpf_verifier_env *env,
> +				   struct bpf_func_state *state, int id)
> +{
> +	struct bpf_reg_state *regs = state->regs, *reg;
> +	int i;
> +
> +	for (i = 0; i < MAX_BPF_REG; i++)
> +		if (regs[i].id == id)
> +			mark_reg_unknown(env, regs, i);
> +
> +	bpf_for_each_spilled_reg(i, state, reg) {
> +		if (!reg)
> +			continue;
> +		if (reg_is_refcounted(reg) && reg->id == id)
> +			__mark_reg_unknown(reg);
> +	}
> +}

Hi Joe,

I've been looking at this function again and wondering why second reg->id == id
check needs additional reg_is_refcounted() check?
No tests have failed when I removed it.
If reg->id is equal to id being released we need to clear the reg regardless
whethere it's in the registers (the first loop) or
whether it's in the stack (second loop).
I think when reg->id == id that reg is guaranteed to be refcounted.
Would you agree?

I propose to simply remove that unnecessary reg_is_refcounted(reg) check.
We can replace it with warn_on, but imo that's overkill.
I'm thinking to repurpose release_reg_references() function for my bpf_spin_lock work
and that check is in the way.