netdev - Re: [PATCH v4 1/2] net/handshake: Create a NETLINK service for handling handshake requests

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <df1c06c2a2b516e4adb5d74cf1f50935e745abdc.camel@redhat.com>
Date:   Thu, 16 Feb 2023 14:12:08 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Chuck Lever <chuck.lever@...cle.com>, kuba@...nel.org,
        edumazet@...gle.com
Cc:     netdev@...r.kernel.org, hare@...e.com, dhowells@...hat.com,
        bcodding@...hat.com, kolga@...app.com, jmeneghi@...hat.com
Subject: Re: [PATCH v4 1/2] net/handshake: Create a NETLINK service for
 handling handshake requests

[partial feedback /me is still a bit lost in the code ;]
On Wed, 2023-02-15 at 14:23 -0500, Chuck Lever wrote:
> +/*
> + * This function is careful to not close the socket. It merely removes
> + * it from the file descriptor table so that it is no longer visible
> + * to the calling process.
> + */
> +static int handshake_genl_cmd_done(struct sk_buff *skb, struct genl_info *gi)
> +{
> +	struct nlattr *tb[HANDSHAKE_GENL_ATTR_MAX + 1];
> +	struct handshake_req *req;
> +	struct socket *sock;
> +	int fd, status, err;
> +
> +	err = genlmsg_parse(nlmsg_hdr(skb), &handshake_genl_family, tb,
> +			    HANDSHAKE_GENL_ATTR_MAX, handshake_genl_policy,
> +			    NULL);
> +	if (err) {
> +		pr_err_ratelimited("%s: genlmsg_parse() returned %d\n",
> +				   __func__, err);
> +		return err;
> +	}
> +
> +	if (!tb[HANDSHAKE_GENL_ATTR_SOCKFD])
> +		return handshake_genl_status_reply(skb, gi, -EINVAL);
> +	err = 0;
> +	fd = nla_get_u32(tb[HANDSHAKE_GENL_ATTR_SOCKFD]);
> +	sock = sockfd_lookup(fd, &err);
> +	if (err)
> +		return handshake_genl_status_reply(skb, gi, -EBADF);
> +
> +	req = sock->sk->sk_handshake_req;
> +	if (req->hr_fd != fd)	/* sanity */
> +		return handshake_genl_status_reply(skb, gi, -EBADF);
> +
> +	status = -EIO;
> +	if (tb[HANDSHAKE_GENL_ATTR_SESS_STATUS])
> +		status = nla_get_u32(tb[HANDSHAKE_GENL_ATTR_SESS_STATUS]);
> +
> +	put_unused_fd(req->hr_fd);

If I read correctly, at this point the user-space is expected to have
already closed hr_fd , but that is not enforced, right? a buggy or
malicious user-space could cause bad things not closing such fd.

Can we use sockfd_put(sock) instead? will make the code more readable,
I think.

BTW I don't think there is any problem with the sock->sk dereference
above, the fd reference count will prevent __sock_release from being
called.

[...]

> +static void __net_exit handshake_net_exit(struct net *net)
> +{
> +	struct handshake_req *req;
> +	LIST_HEAD(requests);
> +
> +	/*
> +	 * XXX: This drains the net's pending list, but does
> +	 *	nothing about requests that have been accepted
> +	 *	and are in progress.
> +	 */
> +	spin_lock(&net->hs_lock);
> +	list_splice_init(&requests, &net->hs_requests);
> +	spin_unlock(&net->hs_lock);

If I read correctly accepted, uncompleted reqs are leaked. I think that
could be prevented installing a custom sk_destructor in sock->sk
tacking care of freeing the sk->sk_handshake_req. The existing/old
sk_destructor - if any - could be stored in an additional
sk_handshake_req field and tail-called by the req's one.

[...]

> +/*
> + * This limit is to prevent slow remotes from causing denial of service.
> + * A ulimit-style tunable might be used instead.
> + */
> +#define HANDSHAKE_PENDING_MAX (10)

I liked the idea of a core mem based limit ;) not a big deal anyway ;)

> +
> +struct handshake_req *handshake_req_get(struct handshake_req *req)
> +{
> +	return likely(refcount_inc_not_zero(&req->hr_ref)) ? req : NULL;
> +}

It's unclear to me under which circumstances the refcount should be >
1: AFAICS the req should have always a single owner: initially the
creator, then the accept queue and finally the user-space serving the
request.

Cheers,

Paolo