Re: [PATCH bpf-next v14 4/8] bpf: Add syscall common attributes support for prog_load

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The log attribute merge logic in bpf_log_attr_init() incorrectly relies solely on the presence of log_buf to detect log parameters, ignoring valid requests from bpf_common_attr and bypassing conflict checks.
--

commit 1f52aae509f179f0117e37fa0cdfc161beb16d35
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Add syscall common attributes support for prog_load

This commit allows BPF_PROG_LOAD to accept log parameters from either union
bpf_attr or struct bpf_common_attr. The merge rules are designed to use
complete log tuples from either side or return an error if they differ.

> diff --git a/kernel/bpf/log.c b/kernel/bpf/log.c
> index 1b1efe75398b7..fd12ad5a0338d 100644
> --- a/kernel/bpf/log.c
> +++ b/kernel/bpf/log.c

[ ... ]

> @@ -827,16 +827,36 @@ void print_insn_state(struct bpf_verifier_env *env, const struct bpf_verifier_st
>  }
>  
>  int bpf_log_attr_init(struct bpf_log_attr *log, u64 log_buf, u32 log_size, u32 log_level,
> -		      u32 offsetof_log_true_size, bpfptr_t uattr)
> +		      u32 offsetof_log_true_size, bpfptr_t uattr, struct bpf_common_attr *common,
> +		      bpfptr_t uattr_common, u32 size_common)
>  {
> +	char __user *ubuf_common = u64_to_user_ptr(common->log_buf);
>  	char __user *ubuf = u64_to_user_ptr(log_buf);
>  
> +	if (!bpf_verifier_log_attr_valid(common->log_level, ubuf_common, common->log_size) ||
> +	    !bpf_verifier_log_attr_valid(log_level, ubuf, log_size))
> +		return -EINVAL;
> +
> +	if (ubuf && ubuf_common && (ubuf != ubuf_common || log_size != common->log_size ||
> +				    log_level != common->log_level))
> +		return -EINVAL;

The commit message states that if both sides provide complete tuples but
they differ, it should return -EINVAL.

If a user wants to query the true log size, they can set log_level > 0
with log_buf = NULL and log_size = 0. This is explicitly permitted by
bpf_verifier_log_attr_valid().

If both attr and attr_common omit the log buffer but provide conflicting
log_level values, will this bypass the conflict check since both ubuf and
ubuf_common evaluate to NULL?

>  	memset(log, 0, sizeof(*log));
>  	log->ubuf = ubuf;
>  	log->size = log_size;
>  	log->level = log_level;
>  	log->offsetof_true_size = offsetof_log_true_size;
>  	log->uattr = uattr;
> +
> +	if (!ubuf && ubuf_common) {

If the user sets common->log_level > 0 to query the true log size but
omits common->log_buf, ubuf_common evaluates to NULL.

Does this cause the condition !ubuf && ubuf_common to evaluate to false,
silently ignoring the valid attr_common request and falling back to the
legacy attr?

Additionally, if both sides provide perfectly matching log buffers, but
the legacy attr is too small to receive log_true_size (where
offsetof_log_true_size is 0), would the code fail to utilize the
attr_common capacity and silently drop the log_true_size output?

> +		log->ubuf = ubuf_common;
> +		log->size = common->log_size;
> +		log->level = common->log_level;
> +		log->uattr = uattr_common;
> +		log->offsetof_true_size = 0;
> +		if (size_common >= offsetofend(struct bpf_common_attr, log_true_size))
> +			log->offsetof_true_size = offsetof(struct bpf_common_attr, log_true_size);
> +	}
>  	return 0;
>  }

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