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: <m2jzhgx6sg.fsf@chopps.org>
Date: Fri, 19 Jul 2024 17:06:41 -0700
From: Christian Hopps <chopps@...pps.org>
To: Florian Westphal <fw@...len.de>
Cc: Christian Hopps <chopps@...pps.org>, 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


Florian Westphal <fw@...len.de> writes:

> 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.

I've switched it to the more obvious array of pointers and storing the module provided pointer.

Thanks,
Chris.

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)