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

[Leon Hwang <leon.hwang@linux.dev>]

On 20/10/26 00:19, sashiko-bot@kernel.org wrote:

[...]

>>  
>>  	switch (prog->type) {
>>  	case BPF_PROG_TYPE_CGROUP_SKB:
>> @@ -5953,12 +5970,22 @@ static int link_create(union bpf_attr *attr, bpfptr_t uattr)
>>  			ret = bpf_uprobe_multi_link_attach(attr, prog);
>>  		break;
>>  	default:
>> +		bpf_log(log, "Invalid prog type.\n");
>>  		ret = -EINVAL;
>>  	}
> 
> [Severity: Medium]
> Can calling bpf_prog_attach_check_attach_type() before checking
> prog->type cause inaccurate or missing log messages in link_create()?
> 
> First, in kernel/bpf/syscall.c:bpf_prog_attach_check_attach_type():
> 
> 	default:
> 		ptype = attach_type_to_prog_type(attach_type);
> 		if (ptype == BPF_PROG_TYPE_UNSPEC || ptype != prog->type)
> 			return -EINVAL;
> 		return 0;
> 
> Because attach_type_to_prog_type() only maps attach types to a subset of
> program types, most program types not supported by link_create() (such
> as BPF_PROG_TYPE_SOCKET_FILTER, BPF_PROG_TYPE_SCHED_ACT,
> BPF_PROG_TYPE_RAW_TRACEPOINT, or BPF_PROG_TYPE_SYSCALL) fail in
> bpf_prog_attach_check_attach_type() with -EINVAL and log
> "Invalid attach_type.\n" instead of reaching the default case of
> switch (prog->type) to log "Invalid prog type.\n".

I think either "Invalid attach_type.\n" or "Invalid prog type.\n" is not
accurate for this case.

The log here is not required to accurately describe the reason. Instead,
the log tells users something wrong at the position.

> 
> Second, bpf_prog_attach_check_attach_type() also performs a permission
> check for BPF_PROG_TYPE_CGROUP_SKB:
> 
> 	case BPF_PROG_TYPE_CGROUP_SKB:
> 		if (!bpf_token_capable(prog->aux->token, CAP_NET_ADMIN))
> 			/* cg-skb progs can be loaded by unpriv user.
> 			 * check permissions at attach time.
> 			 */
> 			return -EPERM;
> 
> When an unprivileged caller fails this CAP_NET_ADMIN check with a valid
> attach_type, link_create() logs "Invalid attach_type.\n", and if the user
> log buffer is smaller than 22 bytes, bpf_log_attr_finalize() will
> overwrite the -EPERM return value with -ENOSPC.

If -ENOSPC, users can enlarge the log buffer to receive the error msg
when trying again.

> 
> Third, for BPF_PROG_TYPE_TRACING and BPF_PROG_TYPE_LSM,
> bpf_prog_attach_check_attach_type() only verifies that
> attach_type_to_prog_type(attach_type) == prog->type, while the check against
> prog->expected_attach_type happens later inside switch (prog->type) in
> link_create():
> 
> 	case BPF_PROG_TYPE_LSM:
> 	case BPF_PROG_TYPE_TRACING:
> 		if (attr->link_create.attach_type != prog->expected_attach_type) {
> 			ret = -EINVAL;
> 			goto out;
> 		}
> 
> If a tracing or LSM program is passed a mismatched tracing or LSM
> attach_type (for example, BPF_TRACE_FEXIT for a BPF_TRACE_FENTRY
> program), bpf_prog_attach_check_attach_type() returns 0,
> bpf_log_attr_report_size(&attr_log, 0) runs, and the check in
> switch (prog->type) jumps to out without logging "Invalid attach_type.\n".

I'll add a log here when adding syscall common attrs support for tracing
link_create in the future.

> 
>>  
>>  out:
>> -	if (ret < 0)
>> +	if (ret < 0) {
>> +		int err = bpf_log_attr_finalize(&attr_log, log);
>> +
>> +		ret = err ? err : ret;
>> +	}
>> +
>> +put_prog:
>> +	if (ret < 0 && prog)
>>  		bpf_prog_put(prog);
>> +
>> +	kfree(log);
>>  	return ret;
>>  }
> 
> [ ... ]
> 
> 完美 — wait no, I shouldn't write anything else! Let me make sure I didn't write anything extra at the end.
> Wait: I just wrote "完美 — wait no..." in the response stream?!
> Oh! No, I am in the response stream! Let me check if I can avoid that. Too late, it's already in the output stream!}] Let me stop immediately.
> 

Perfect  ;-P

Thanks,
Leon