netdev - Re: [PATCH bpf-next v5 2/7] bpf: Support associating BPF program with struct

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: <e9468bb9f2cc62c69d9364a4ce2ab5ee08fafa6576d6be6a121b04a80a379094@mail.kernel.org>
Date: Tue,  4 Nov 2025 17:54:54 +0000 (UTC)
From: bot+bpf-ci@...nel.org
To: ameryhung@...il.com,bpf@...r.kernel.org
Cc: netdev@...r.kernel.org,alexei.starovoitov@...il.com,andrii@...nel.org,daniel@...earbox.net,tj@...nel.org,martin.lau@...nel.org,ameryhung@...il.com,kernel-team@...a.com,ast@...nel.org,andrii@...nel.org,daniel@...earbox.net,martin.lau@...nel.org,eddyz87@...il.com,yonghong.song@...ux.dev,clm@...a.com,ihor.solodrai@...ux.dev
Subject: Re: [PATCH bpf-next v5 2/7] bpf: Support associating BPF program with struct_ops

> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index a47d67db3..0f71030c0 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h

[ ... ]

> @@ -2026,6 +2028,9 @@ static inline void bpf_module_put(const void *data, struct module *owner)
>  		module_put(owner);
>  }
>  int bpf_struct_ops_link_create(union bpf_attr *attr);
> +int bpf_prog_assoc_struct_ops(struct bpf_prog *prog, struct bpf_map *map);
> +void bpf_prog_disassoc_struct_ops(struct bpf_prog *prog);
> +void *bpf_prog_get_assoc_struct_ops(const struct bpf_prog_aux *aux);
>  u32 bpf_struct_ops_id(const void *kdata);

[ ... ]

> diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
> index a41e6730e..0a19842da 100644
> --- a/kernel/bpf/bpf_struct_ops.c
> +++ b/kernel/bpf/bpf_struct_ops.c

[ ... ]

> @@ -1394,6 +1413,77 @@ int bpf_struct_ops_link_create(union bpf_attr *attr)
>  	return err;
>  }
>
> +int bpf_prog_assoc_struct_ops(struct bpf_prog *prog, struct bpf_map *map)
> +{
> +	struct bpf_map *st_ops_assoc;
> +
> +	guard(mutex)(&prog->aux->st_ops_assoc_mutex);
> +
> +	st_ops_assoc = prog->aux->st_ops_assoc;
> +
> +	if (st_ops_assoc && st_ops_assoc == map)
> +		return 0;
> +
> +	if (st_ops_assoc) {
> +		if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> +			return -EBUSY;
> +
> +		WRITE_ONCE(prog->aux->st_ops_assoc, BPF_PTR_POISON);
> +	} else {
> +		if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> +			bpf_map_inc(map);
> +
> +		WRITE_ONCE(prog->aux->st_ops_assoc, map);
> +	}
> +
> +	return 0;
> +}
> +
> +void bpf_prog_disassoc_struct_ops(struct bpf_prog *prog)
> +{
> +	struct bpf_map *st_ops_assoc;
> +
> +	guard(mutex)(&prog->aux->st_ops_assoc_mutex);
> +
> +	st_ops_assoc = prog->aux->st_ops_assoc;
> +
> +	if (!st_ops_assoc || st_ops_assoc == BPF_PTR_POISON)
> +		return;
> +
> +	if (prog->type != BPF_PROG_TYPE_STRUCT_OPS)
> +		bpf_map_put(st_ops_assoc);
> +
> +	WRITE_ONCE(prog->aux->st_ops_assoc, NULL);
> +}
> +
> +/*
> + * Get a reference to the struct_ops struct (i.e., kdata) associated with a
> + * program.
> + *
> + * If the returned pointer is not NULL, it must points to a valid and
> + * initialized struct_ops. The struct_ops may or may not be attached.
> + * Kernel struct_ops implementers are responsible for tracking and checking
> + * the state of the struct_ops if the use case requires an attached struct_ops.
> + */
> +void *bpf_prog_get_assoc_struct_ops(const struct bpf_prog_aux *aux)
> +{
> +	struct bpf_map *st_ops_assoc = READ_ONCE(aux->st_ops_assoc);
> +	struct bpf_struct_ops_map *st_map;
> +
> +	if (!st_ops_assoc || st_ops_assoc == BPF_PTR_POISON)
> +		return NULL;
> +
> +	st_map = (struct bpf_struct_ops_map *)st_ops_assoc;
> +
> +	if (smp_load_acquire(&st_map->kvalue.common.state) == BPF_STRUCT_OPS_STATE_INIT) {
> +		bpf_map_put(st_ops_assoc);
                ^^^^

Does bpf_prog_get_assoc_struct_ops() need to call bpf_map_put() here?

The function comment says "Get a reference to the struct_ops struct"
but the function never calls bpf_map_get/inc() to acquire a reference.
It only reads aux->st_ops_assoc via READ_ONCE().

When the state check fails (INIT state), the function calls bpf_map_put()
which drops the reference that was acquired in bpf_prog_assoc_struct_ops().
But on the success path below, it returns kdata without any refcount
operation.

This creates an imbalance:
- INIT state path: drops a reference, returns NULL
- Non-INIT path: no refcount change, returns kdata

The caller has no way to know whether the reference count was modified.
Also, the function is EXPORTED but doesn't document the reference counting
semantics clearly.

> +		return NULL;
> +	}
> +
> +	return &st_map->kvalue.data;
> +}
> +EXPORT_SYMBOL_GPL(bpf_prog_get_assoc_struct_ops);

Can this function race with bpf_prog_disassoc_struct_ops()?

Since bpf_prog_get_assoc_struct_ops() reads aux->st_ops_assoc via
READ_ONCE() without holding the mutex or acquiring a reference, there's
a window where:

Thread A: reads st_ops_assoc pointer
Thread B: calls bpf_prog_disassoc_struct_ops()->bpf_map_put() (drops last ref)
Thread A: dereferences st_map->kvalue.common.state (use-after-free)

For non-struct_ops programs, bpf_prog_assoc_struct_ops() holds a
reference via bpf_map_inc(). This reference is dropped in
bpf_prog_disassoc_struct_ops() when the program is freed. Without
acquiring a new reference in bpf_prog_get_assoc_struct_ops(), the map
can be freed during the race window above.

[ ... ]


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/19077679684