netdev - Re: [devel-ipsec] [PATCH ipsec-next v5 06/17] xfrm: add mode

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: <20240719061413.GA29401@breakpoint.cc>
Date: Fri, 19 Jul 2024 08:14:13 +0200
From: Florian Westphal <fw@...len.de>
To: Christian Hopps <chopps@...pps.org>
Cc: devel@...ux-ipsec.org, Steffen Klassert <steffen.klassert@...unet.com>,
	netdev@...r.kernel.org, Christian Hopps <chopps@...n.net>
Subject: Re: [devel-ipsec] [PATCH ipsec-next v5 06/17] xfrm: add mode_cbs
 module functionality

Christian Hopps via Devel <devel@...ux-ipsec.org> wrote:
> +static struct xfrm_mode_cbs xfrm_mode_cbs_map[XFRM_MODE_MAX];

Why not
static struct xfrm_mode_cbs *xfrm_mode_cbs_map[XFRM_MODE_MAX];
i.e., why does

> +int xfrm_register_mode_cbs(u8 mode, const struct xfrm_mode_cbs *mode_cbs)
> +{
> +	if (mode >= XFRM_MODE_MAX)
> +		return -EINVAL;
> +
> +	xfrm_mode_cbs_map[mode] = *mode_cbs;

do a deep copy here rather than
	xfrm_mode_cbs_map[mode] = mode_cbs;

?

> +	if (mode == XFRM_MODE_IPTFS && !xfrm_mode_cbs_map[mode].create_state)
> +		request_module("xfrm-iptfs");
> +	return &xfrm_mode_cbs_map[mode];

Sabrina noticed that this looks weird, and I agree.

This can, afaics, return a partially initialised structure or an
all-zero structure.

request_module() might load the module, it can fail to load the
module, and, most importantly, there is no guarantee that the
module is removed right after.

Is there any locking scheme in place that prevents this from
happening? If so, a comment should be added that explains this.