Florian Westphal [off-list ref] writes:

Christian Hopps [off-list ref] wrote:

quoted

+static int __iptfs_init_state(struct xfrm_state *x,
+			      struct xfrm_iptfs_data *xtfs)
+{
+	/* Modify type (esp) adjustment values */
+
+	if (x->props.family == AF_INET)
+		x->props.header_len += sizeof(struct iphdr) + sizeof(struct ip_iptfs_hdr);
+	else if (x->props.family == AF_INET6)
+		x->props.header_len += sizeof(struct ipv6hdr) + sizeof(struct ip_iptfs_hdr);
+	x->props.enc_hdr_len = sizeof(struct ip_iptfs_hdr);
+
+	/* Always have a module reference if x->mode_data is set */
+	if (!try_module_get(x->mode_cbs->owner))
+		return -EINVAL;

If the comment means that we already have a module owner ref taken
before this try_module_get, then this should use __module_get and
a mention where the first ref was taken.

If not, then this needs an explanation as to what prevents another cpu to
rmmod the owning module between the lookup in xfrm_init_state and the
module reference in __iptfs_init_state.

cpu0					cpu1
 xfrm_init_state
   -> xfrm_get_mode_cbs                 rmmod
     -> __iptfs_init_state		  xfrm_iptfs_fini
       <interrupt>                         xfrm_unregister_mode_cbs
                                            release memory
       <resume>
	try_module_get -> UaF

You are correct the code is not sufficiently protective.

I need to use rcu to keep `xfrm_mode_cbs_map[mode]` around long enough to do a `try_module_get(&xfrm_mode_cbs_map[mode]->owner)` and return from `xfrm_get_mode_cbs()` with that ref count held. The caller (xfrm_init_state()) will then need do a `module_put()` after it calls `mode_cbs->create_state()` to give the reference back.

Thanks,
Chris.