Re: [PATCH net v2 1/1] ipv6: flowlabel: cap duplicate leases per socket

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
ipv6: flowlabel: cap duplicate leases per socket

This adds a duplicate-lease counter to the socket-list walk in
ipv6_flowlabel_get() and rejects a new unprivileged lease with -ENOBUFS
once the count reaches FL_MAX_PER_SOCK, before linger and expires are
refreshed.

A few questions about the new counting and the capability query below.

> diff --git a/net/ipv6/ip6_flowlabel.c b/net/ipv6/ip6_flowlabel.c
> index 1ab5ad0dcf24..55cc5ec485c5 100644
> --- a/net/ipv6/ip6_flowlabel.c
> +++ b/net/ipv6/ip6_flowlabel.c
> @@ -617,6 +617,8 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq,
>  	struct ipv6_fl_socklist *sfl, *sfl1 = NULL;
>  	struct ip6_flowlabel *fl, *fl1 = NULL;
>  	struct net *net = sock_net(sk);
> +	bool cap_net_admin = false;
> +	int dup_count = 0;
>  	int err;
>  
>  	if (freq->flr_flags & IPV6_FL_F_REFLECT) {
> @@ -652,10 +654,14 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq,
>  					rcu_read_unlock();
>  					goto done;
>  				}
> -				fl1 = sfl->fl;
> -				if (!atomic_inc_not_zero(&fl1->users))
> -					fl1 = NULL;
> -				break;
> +				if (!fl1) {
> +					fl1 = sfl->fl;
> +					if (!atomic_inc_not_zero(&fl1->users))
> +						fl1 = NULL;
> +					cap_net_admin = capable(CAP_NET_ADMIN);

[Severity: Medium]
Is it intentional that capable(CAP_NET_ADMIN) now runs on the normal
success path of every reuse GET?

The only consumer is the later test:

	err = -ENOBUFS;
	if (dup_count >= FL_MAX_PER_SOCK && !cap_net_admin)
		goto release;

which is only reachable once dup_count has saturated at FL_MAX_PER_SOCK,
so for a GET that succeeds with dup_count between 1 and 31 the result is
never read.

capable() is the auditing variant:

kernel/capability.c:capable() -> ns_capable() ->
	ns_capable_common(ns, cap, CAP_OPT_NONE) -> security_capable()

and SELinux audits whenever CAP_OPT_NOAUDIT is not set:

security/selinux/hooks.c:cred_has_capability() {
	...
	if (!(opts & CAP_OPT_NOAUDIT)) {
		int rc2 = avc_audit(sid, sid, sclass, av, &avd, rc, &ad);
	...
}

So does an unprivileged process looping
setsockopt(IPV6_FLOWLABEL_MGR, IPV6_FL_A_GET) on a label it already holds
now emit one "denied { net_admin }" record per syscall, on a path that
previously performed no capability check at all?  capable() also sets
PF_SUPERPRIV for privileged callers that never end up using the privilege.

Since the "if (!fl1)" block is re-entered whenever atomic_inc_not_zero()
failed, can this also run more than once per syscall?

Would evaluating the capability at the rejection site, or using a
*_noaudit variant for this speculative query, give the same policy without
success-path side effects?  Note the "|| cap_net_admin" loop shortcut just
below depends on the eager evaluation, so the ordering would need reworking
rather than just moving the call.

> +				}
> +				if (++dup_count >= FL_MAX_PER_SOCK || cap_net_admin)
> +					break;

[Severity: Medium]
This isn't a bug introduced by this patch, but a question about how much
of the pre-existing growth path the new counter covers.

dup_count is only incremented inside the "sfl->fl->label ==
freq->flr_label" branch, so the new limit is per (socket, label).  The
pre-existing use of the same constant in mem_check() bounds the total
number of lease entries on the socket:

	rcu_read_lock();
	for_each_sk_fl_rcu(sk, sfl)
		count++;
	rcu_read_unlock();

	if (room <= 0 ||
	    ((count >= FL_MAX_PER_SOCK ||
	...

Is FL_MAX_PER_SOCK now expressing two different policies, and would a
separate name for the per-label limit be clearer?

Also, the reuse path reached through fl_lookup():

		if (!fl1)
			fl1 = fl_lookup(net, freq->flr_label);
		if (fl1) {
recheck:

arrives at recheck with dup_count == 0 and never consults mem_check(), so
the first lease of each distinct globally interned shareable label is still
admitted with no accounting.  Can an unprivileged caller still grow one
socket's lease list to roughly FL_MAX_PER_SOCK times the number of
shareable labels reachable in the netns, each node pinning an
ip6_flowlabel and lengthening the O(n) socket-list walks?

The residual looks bounded in practice: mem_check() refuses new-label
creation for an unprivileged socket once fl_size >= FL_MAX_PER_SOCK, and
net->ipv6.flowlabel_count is capped at unpriv_user_limit (~3072) per netns,
which works out to roughly 3 MB of ipv6_fl_socklist nodes per socket
reached via ~10^5 setsockopt calls.  Before the patch a single label could
be leased an unbounded number of times, so this is an improvement; would
accounting the whole lease list the way mem_check() does close the rest?

>  			}
>  		}
>  		rcu_read_unlock();
> @@ -679,6 +685,10 @@ static int ipv6_flowlabel_get(struct sock *sk, struct in6_flowlabel_req *freq,
>  			err = -ENOMEM;
>  			if (!sfl1)
>  				goto release;
> +			/* sockopt_lock_sock() serializes the count and fl_link(). */
> +			err = -ENOBUFS;
> +			if (dup_count >= FL_MAX_PER_SOCK && !cap_net_admin)
> +				goto release;

[Severity: Low]
Could a selftest be added for this new -ENOBUFS return?  The existing
in-tree contract test cannot reach it.

tools/testing/selftests/net/ipv6_flowlabel_mgr.c:run_tests() performs only
three duplicate GETs of label 1 followed by three PUTs:

	explain("can now put exactly three references");
	expect_pass(flowlabel_put(fd, 1));

which is well below FL_MAX_PER_SOCK, and the wrapper runs it as root:

tools/testing/selftests/net/ipv6_flowlabel.sh:
	./in_netns.sh ./ipv6_flowlabel_mgr

so capable(CAP_NET_ADMIN) is true and the walk breaks after the first
match with dup_count == 1.  As it stands, the test would still pass if the
cap were removed, off by one, or rejected the first duplicate.  Exercising
it would need more than 32 duplicate GETs after dropping CAP_NET_ADMIN,
similar to what the existing IPV6_FL_S_USER case already does in a setuid
child.

>  			if (fl->linger > fl1->linger)
>  				fl1->linger = fl->linger;
>  			if ((long)(fl->expires - fl1->expires) > 0)
-- 
This is an AI-generated review.