Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] bpf: incorrect error log messages and ordering in link_create() --
commit f2293d2ca04aeb83f219d17a0904b9b58058c572 Author: Leon Hwang <[email protected]> bpf: Add syscall common attrs support for link_create This commit adds syscall common attributes support to link_create(), creating and finalizing a verifier log instance to report errors for invalid prog_fd, attach_type, and prog type. Link: https://lore.kernel.org/bpf/[email protected]/ > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c > index 90bc7230afa8a..db870e795d044 100644 > --- a/kernel/bpf/syscall.c > +++ b/kernel/bpf/syscall.c > @@ -5862,8 +5862,11 @@ 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; > > @@ -5873,14 +5876,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: > @@ -5959,12 +5972,20 @@ 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 switching on prog->type cause link_create() to log the wrong error message or skip logging on mismatched attach types? First, in kernel/bpf/syscall.c:bpf_prog_attach_check_attach_type(), a cgroup skb program checks permissions before validating the attach type: 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 that capability check fails, link_create() still logs "Invalid attach_type.\n" even though attach_type may be valid. Second, for program types that are not supported by link_create() and have no mapping in attach_type_to_prog_type() (such as socket filter, sched act, raw tracepoint, or syscall programs), bpf_prog_attach_check_attach_type() hits its default branch and returns -EINVAL before link_create() reaches switch (prog->type): default: ptype = attach_type_to_prog_type(attach_type); if (ptype == BPF_PROG_TYPE_UNSPEC || ptype != prog->type) return -EINVAL; return 0; Doesn't that cause link_create() to log "Invalid attach_type.\n" and jump to out, leaving the new default branch that logs "Invalid prog type.\n" unreachable for almost all unsupported program types? Third, for tracing and lsm programs, bpf_prog_attach_check_attach_type() only checks that attach_type maps to prog->type in attach_type_to_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; } Should that mismatch also log "Invalid attach_type.\n"? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
