netdev - Re: [PATCH net-next v2 2/8] netfilter: add API to manage NAT helpers.

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: <20190415054820.p2vbw7itmr7gwmq5@salvia>
Date:   Mon, 15 Apr 2019 07:48:20 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Flavio Leitner <fbl@...hat.com>
Cc:     netdev@...r.kernel.org, Joe Stringer <joe@....org>,
        Pravin B Shelar <pshelar@....org>, dev@...nvswitch.org,
        netfilter-devel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/8] netfilter: add API to manage NAT helpers.

Sorry I didn't see this in the first review.

On Sat, Apr 13, 2019 at 08:17:10PM -0300, Flavio Leitner wrote:
[...]
> +int
> +nf_nat_helper_try_module_get(const char *name, u16 l3num, u8 protonum)
> +{
> +	struct nf_conntrack_helper *h;
> +	struct nf_conntrack_nat_helper *nat;
> +	char mod_name[NF_CT_HELPER_NAME_LEN];
> +	int ret = 0;
> +
> +	rcu_read_lock();
> +	h = __nf_conntrack_helper_find(name, l3num, protonum);
> +	if (h == NULL) {
> +		rcu_read_unlock();
> +		return -EINVAL;
> +	}
> +
> +	if (!strlen(h->nat_mod_name)) {
> +		rcu_read_unlock();
> +		return -EOPNOTSUPP;

Probably check for this at registration?

> +	}
> +
> +	nat = nf_conntrack_nat_helper_find(h->nat_mod_name);
> +	if (nat == NULL) {
> +		snprintf(mod_name, sizeof(mod_name), "%s", h->nat_mod_name);
> +		rcu_read_unlock();
> +		ret = request_module(mod_name);
> +		if (ret != 0)
> +			return ret;

Not sure it is worth checking for request_module() return value, the
code just below already is doing this.

> +
> +		rcu_read_lock();
> +		nat = nf_conntrack_nat_helper_find(mod_name);
> +		if (nat == NULL) {
> +			rcu_read_unlock();
> +			return -EINVAL;

ENOENT?

> +		}
> +	}
> +
> +	if (!try_module_get(nat->module))
> +		ret = -EINVAL;

ENOENT?

Telling this because we will at some point propagate this error value
to userspace by when we start using this infrastructure you're working
on. EINVAL is already very overload in netlink and we'll use it from
there.

> +
> +	rcu_read_unlock();
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nf_nat_helper_try_module_get);