Re: [PATCH bpf-next 1/6] bpf: Add syscall common attrs support for link_create

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] bpf: prog refcount underflow and fd leak in link_create() on log error
--

commit 56fa3f4abf91b9651c1cd02884b19754bd9ebcb5
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Add syscall common attrs support for link_create

This commit adds syscall common attributes support to link_create(),
creating and finalizing an internal verifier log instance and reporting
error messages for an invalid prog_fd or attach_type.

Link: https://lore.kernel.org/bpf/20260514004634.3DA09C19425@smtp.kernel.org/

> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 0f27604a54aae..252b479937a4d 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -5839,10 +5839,13 @@ static int bpf_map_do_batch(const union bpf_attr *attr,
>  }
>  
>  #define BPF_LINK_CREATE_LAST_FIELD link_create.uprobe_multi.path_fd
> -static int link_create(union bpf_attr *attr, bpfptr_t uattr)
> +static int link_create(union bpf_attr *attr, bpfptr_t uattr, struct bpf_common_attr *attr_common,
> +		       bpfptr_t uattr_common, u32 size_common)
>  {
> +	struct bpf_verifier_log *log;
> +	struct bpf_log_attr attr_log;
>  	struct bpf_prog *prog;
> -	int ret;
> +	int ret, err;
>  
>  	if (CHECK_ATTR(BPF_LINK_CREATE))
>  		return -EINVAL;
> @@ -5850,14 +5853,24 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
>  	if (attr->link_create.attach_type == BPF_STRUCT_OPS)
>  		return bpf_struct_ops_link_create(attr);
>  
> +	log = bpf_log_attr_create_vlog(&attr_log, attr_common, uattr_common, size_common);
> +	if (IS_ERR(log))
> +		return PTR_ERR(log);
> +
>  	prog = bpf_prog_get(attr->link_create.prog_fd);
> -	if (IS_ERR(prog))
> -		return PTR_ERR(prog);
> +	if (IS_ERR(prog)) {
> +		bpf_log(log, "Invalid prog_fd.\n");
> +		ret = PTR_ERR(prog);
> +		prog = NULL;
> +		goto out;
> +	}
>  
>  	ret = bpf_prog_attach_check_attach_type(prog,
>  						attr->link_create.attach_type);
> -	if (ret)
> +	if (ret) {
> +		bpf_log(log, "Invalid attach_type.\n");
>  		goto out;
> +	}
>  
>  	switch (prog->type) {
>  	case BPF_PROG_TYPE_CGROUP_SKB:
> @@ -5940,7 +5953,14 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
>  	}
>  
>  out:
> -	if (ret < 0)
> +	/* preserve original error even if log finalization is successful */
> +	err = bpf_log_attr_finalize(&attr_log, log);
> +	if (err)
> +		ret = err;
> +
> +	kfree(log);
> +
> +	if (ret < 0 && prog)
>  		bpf_prog_put(prog);
>  	return ret;
>  }

[Severity: Critical]
Can this underflow the refcount on prog and leak the newly installed link
file descriptor if bpf_log_attr_finalize() fails after link attachment
succeeds?

When one of the link attach helpers in switch (prog->type) succeeds, it
transfers ownership of the prog reference acquired by bpf_prog_get() to
link->prog and calls bpf_link_settle() (or bpf_link_new_fd()), which
installs the file descriptor via fd_install() and returns the non-negative
fd in ret.

At the out label, bpf_log_attr_finalize() is then called:

kernel/bpf/log.c:bpf_log_attr_finalize() {
	err = bpf_vlog_finalize(log, &log_true_size);

	if (attr->offsetof_true_size &&
	    copy_to_bpfptr_offset(attr->uattr, attr->offsetof_true_size, &log_true_size,
				  sizeof(log_true_size)))
		return -EFAULT;

	return err;
}

If uattr_common was passed with size_common large enough to include
log_true_size and resides in read-only memory (or is unmapped before
finalization), copy_to_bpfptr_offset() returns -EFAULT even when nothing
was logged.

In link_create(), ret is then overwritten with -EFAULT, causing
if (ret < 0 && prog) to call bpf_prog_put(prog) on the reference that is
now owned by the active bpf_link. When the installed link fd is later
closed, bpf_link_free() -> bpf_link_dealloc() will call
bpf_prog_put(link->prog) a second time:

kernel/bpf/syscall.c:bpf_link_dealloc() {
	if (link->prog)
		bpf_prog_put(link->prog);
	...
}

Should the ret < 0 check for bpf_prog_put(prog) happen before
ret is overwritten by err from bpf_log_attr_finalize(), or should log
finalization be handled before settling the link?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017142649.51047-1-leon.hwang@linux.dev?part=1