netdev - Re: [bpf PATCH v4 1/3] net: add a UID to use for ULP socket assignment

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <1bbaeb57-585c-a904-7fc2-324f488546f5@iogearbox.net>
Date:   Tue, 6 Feb 2018 13:26:50 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     ast@...nel.org, davejwatson@...com, netdev@...r.kernel.org
Subject: Re: [bpf PATCH v4 1/3] net: add a UID to use for ULP socket
 assignment

On 02/05/2018 07:17 PM, John Fastabend wrote:
[...]
> @@ -124,6 +145,34 @@ int tcp_set_ulp(struct sock *sk, const char *name)
>  	if (!ulp_ops)
>  		return -ENOENT;
>  
> +	if (!ulp_ops->user_visible) {
> +		module_put(ulp_ops->owner);
> +		return -ENOENT;
> +	}
> +
> +	err = ulp_ops->init(sk);
> +	if (err) {
> +		module_put(ulp_ops->owner);
> +		return err;
> +	}
> +
> +	icsk->icsk_ulp_ops = ulp_ops;
> +	return 0;
> +}
> +
> +int tcp_set_ulp_id(struct sock *sk, int ulp)
> +{
> +	struct inet_connection_sock *icsk = inet_csk(sk);
> +	const struct tcp_ulp_ops *ulp_ops;
> +	int err;
> +
> +	if (icsk->icsk_ulp_ops)
> +		return -EEXIST;
> +
> +	ulp_ops = __tcp_ulp_lookup(ulp);
> +	if (!ulp_ops)
> +		return -ENOENT;
> +
>  	err = ulp_ops->init(sk);
>  	if (err) {

Just some minor feedback; v5 would have been impractical for just this
as the fix looks otherwise fine, but I thought I'd mention it here for a
possible follow-up cleanup to address:

tcp_set_ulp() and tcp_set_ulp_id() are quite different from API PoV despite
that they almost look the same: tcp_set_ulp() respects user_visible bool
while tcp_set_ulp_id() ignores it.

I'm wondering, could we somehow make this more obvious from the API itself?

Perhaps:

  int tcp_set_ulp_id(struct sock *sk, int ulp, bool user_visible_only)

And we do:

	[...]
	if (user_visible_only && !ulp_ops->user_visible) {
		module_put(ulp_ops->owner);
		return -ENOENT;
	}

Maybe something like this would make it more obvious for callers, potentially
same could be added in tcp_set_ulp(), so both APIs only differ in name vs id
but not much more (yeah, one pulls in the module as well, but aside from that).
Then, they could also be consolidated, since the only thing different left
is __tcp_ulp_find_autoload() vs __tcp_ulp_lookup() to fetch the actual ulp_ops.
Wdyt?

Thanks,
Daniel