On 20/10/26 00:19, [email protected] 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
