netdev - Re: [PATCH bpf-next v3 1/4] bpf: remove __rcu annotations from bpf_prog

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: <20190528185605.GA20578@tower.DHCP.thefacebook.com>
Date:   Tue, 28 May 2019 18:56:15 +0000
From:   Roman Gushchin <guro@...com>
To:     Stanislav Fomichev <sdf@...gle.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next v3 1/4] bpf: remove __rcu annotations from
 bpf_prog_array

On Tue, May 28, 2019 at 11:29:43AM -0700, Stanislav Fomichev wrote:
> Drop __rcu annotations and rcu read sections from bpf_prog_array
> helper functions. They are not needed since all existing callers
> call those helpers from the rcu update side while holding a mutex.
> This guarantees that use-after-free could not happen.
> 
> In the next patches I'll fix the callers with missing
> rcu_dereference_protected to make sparse/lockdep happy, the proper
> way to use these helpers is:
> 
> 	struct bpf_prog_array __rcu *progs = ...;
> 	struct bpf_prog_array *p;
> 
> 	mutex_lock(&mtx);
> 	p = rcu_dereference_protected(progs, lockdep_is_held(&mtx));
> 	bpf_prog_array_length(p);
> 	bpf_prog_array_copy_to_user(p, ...);
> 	bpf_prog_array_delete_safe(p, ...);
> 	bpf_prog_array_copy_info(p, ...);
> 	bpf_prog_array_copy(p, ...);
> 	bpf_prog_array_free(p);
> 	mutex_unlock(&mtx);
> 
> No functional changes! rcu_dereference_protected with lockdep_is_held
> should catch any cases where we update prog array without a mutex
> (I've looked at existing call sites and I think we hold a mutex
> everywhere).
> 
> Motivation is to fix sparse warnings:
> kernel/bpf/core.c:1803:9: warning: incorrect type in argument 1 (different address spaces)
> kernel/bpf/core.c:1803:9:    expected struct callback_head *head
> kernel/bpf/core.c:1803:9:    got struct callback_head [noderef] <asn:4> *
> kernel/bpf/core.c:1877:44: warning: incorrect type in initializer (different address spaces)
> kernel/bpf/core.c:1877:44:    expected struct bpf_prog_array_item *item
> kernel/bpf/core.c:1877:44:    got struct bpf_prog_array_item [noderef] <asn:4> *
> kernel/bpf/core.c:1901:26: warning: incorrect type in assignment (different address spaces)
> kernel/bpf/core.c:1901:26:    expected struct bpf_prog_array_item *existing
> kernel/bpf/core.c:1901:26:    got struct bpf_prog_array_item [noderef] <asn:4> *
> kernel/bpf/core.c:1935:26: warning: incorrect type in assignment (different address spaces)
> kernel/bpf/core.c:1935:26:    expected struct bpf_prog_array_item *[assigned] existing
> kernel/bpf/core.c:1935:26:    got struct bpf_prog_array_item [noderef] <asn:4> *
> 
> v2:
> * remove comment about potential race; that can't happen
>   because all callers are in rcu-update section
> 
> Cc: Roman Gushchin <guro@...com>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>

Acked-by: Roman Gushchin <guro@...com>

Thanks!