ACK: [SRU][J][PATCH 1/1] bpf: Check rcu_read_lock_trace_held() before calling bpf map helpers

[Roxana Nicolescu]

On 24/09/2024 17:29, Massimiliano Pellizzer wrote:
> From: Hou Tao <houtao1 at huawei.com>
>
> [ Upstream commit 169410eba271afc9f0fb476d996795aa26770c6d ]
>
> These three bpf_map_{lookup,update,delete}_elem() helpers are also
> available for sleepable bpf program, so add the corresponding lock
> assertion for sleepable bpf program, otherwise the following warning
> will be reported when a sleepable bpf program manipulates bpf map under
> interpreter mode (aka bpf_jit_enable=0):
>
>    WARNING: CPU: 3 PID: 4985 at kernel/bpf/helpers.c:40 ......
>    CPU: 3 PID: 4985 Comm: test_progs Not tainted 6.6.0+ #2
>    Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) ......
>    RIP: 0010:bpf_map_lookup_elem+0x54/0x60
>    ......
>    Call Trace:
>     <TASK>
>     ? __warn+0xa5/0x240
>     ? bpf_map_lookup_elem+0x54/0x60
>     ? report_bug+0x1ba/0x1f0
>     ? handle_bug+0x40/0x80
>     ? exc_invalid_op+0x18/0x50
>     ? asm_exc_invalid_op+0x1b/0x20
>     ? __pfx_bpf_map_lookup_elem+0x10/0x10
>     ? rcu_lockdep_current_cpu_online+0x65/0xb0
>     ? rcu_is_watching+0x23/0x50
>     ? bpf_map_lookup_elem+0x54/0x60
>     ? __pfx_bpf_map_lookup_elem+0x10/0x10
>     ___bpf_prog_run+0x513/0x3b70
>     __bpf_prog_run32+0x9d/0xd0
>     ? __bpf_prog_enter_sleepable_recur+0xad/0x120
>     ? __bpf_prog_enter_sleepable_recur+0x3e/0x120
>     bpf_trampoline_6442580665+0x4d/0x1000
>     __x64_sys_getpgid+0x5/0x30
>     ? do_syscall_64+0x36/0xb0
>     entry_SYSCALL_64_after_hwframe+0x6e/0x76
>     </TASK>
>
> Signed-off-by: Hou Tao <houtao1 at huawei.com>
> Link: https://lore.kernel.org/r/20231204140425.1480317-2-houtao@huaweicloud.com
> Signed-off-by: Alexei Starovoitov <ast at kernel.org>
> Signed-off-by: Sasha Levin <sashal at kernel.org>
> (backported from commit d6d6fe4bb105595118f12abeed4a7bdd450853f3 linux-6.1.y)
> [mpellizzer: added the missing header rcupdate_trace.h]
> CVE-2023-52621
> Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> ---
>   kernel/bpf/helpers.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 273f2f0deb23..026f3a5e71a6 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -15,6 +15,7 @@
>   #include <linux/pid_namespace.h>
>   #include <linux/proc_ns.h>
>   #include <linux/security.h>
> +#include <linux/rcupdate_trace.h>
>   
>   #include "../../lib/kstrtox.h"
>   
> @@ -24,12 +25,13 @@
>    *
>    * Different map implementations will rely on rcu in map methods
>    * lookup/update/delete, therefore eBPF programs must run under rcu lock
> - * if program is allowed to access maps, so check rcu_read_lock_held in
> - * all three functions.
> + * if program is allowed to access maps, so check rcu_read_lock_held() or
> + * rcu_read_lock_trace_held() in all three functions.
>    */
>   BPF_CALL_2(bpf_map_lookup_elem, struct bpf_map *, map, void *, key)
>   {
> -	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> +	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held() &&
> +		     !rcu_read_lock_bh_held());
>   	return (unsigned long) map->ops->map_lookup_elem(map, key);
>   }
>   
> @@ -45,7 +47,8 @@ const struct bpf_func_proto bpf_map_lookup_elem_proto = {
>   BPF_CALL_4(bpf_map_update_elem, struct bpf_map *, map, void *, key,
>   	   void *, value, u64, flags)
>   {
> -	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> +	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held() &&
> +		     !rcu_read_lock_bh_held());
>   	return map->ops->map_update_elem(map, key, value, flags);
>   }
>   
> @@ -62,7 +65,8 @@ const struct bpf_func_proto bpf_map_update_elem_proto = {
>   
>   BPF_CALL_2(bpf_map_delete_elem, struct bpf_map *, map, void *, key)
>   {
> -	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_bh_held());
> +	WARN_ON_ONCE(!rcu_read_lock_held() && !rcu_read_lock_trace_held() &&
> +		     !rcu_read_lock_bh_held());
>   	return map->ops->map_delete_elem(map, key);
>   }
>   
Acked-by: Roxana Nicolescu <roxana.nicolescu at canonical.com>