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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The log parameters merge logic incorrectly equates the presence of log parameters with non-NULL log buffers, silently ignoring valid `log_level` configurations and bypassing conflict checks.
- [Medium] When both specific and common attributes provide identical log parameters, `log_true_size` writeback may be lost if the specific `attr` is undersized.
--

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

bpf: Add syscall common attributes support for prog_load

BPF_PROG_LOAD can now take log parameters from both union bpf_attr and
struct bpf_common_attr. The merge rules are: match, use one, or return
-EINVAL.

> 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;

If a user specifies log_level != 0 but leaves log_buf == NULL to probe for
log_true_size, will these checks bypass conflicts?

If one side provides a complete tuple and the other provides only log_level,
or if both provide mismatched log_levels without buffers, it appears the
conflict checks are skipped and no -EINVAL is returned.

> +
>  	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) {

Does this condition silently ignore valid common->log_level configurations?

If the user provides only common->log_level via bpf_common_attr for a log
size dry-run, ubuf_common evaluates to NULL. The condition here would
evaluate to false, and common->log_level would be ignored instead of
applied.

Also, if both specific and common attributes provide identical log
parameters, the code appears to unconditionally use uattr for the
log_true_size writeback since ubuf is not NULL.

If the original uattr uses an older layout without space for log_true_size,
log->offsetof_true_size will be set to 0. Does this mean log_true_size
writeback will be lost even if the user provided a modern attr_common
struct with sufficient space to receive it?

> +		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/20260511152817.89191-1-leon.hwang@linux.dev?part=4