[PATCH net] xfrm: iptfs: only publish mode_data after clone setup

[PATCH net] xfrm: iptfs: only publish mode_data after clone setup

[Paul Moses]

iptfs_clone_state() stores x->mode_data before allocating the reorder
window. If that allocation fails, the code frees the cloned state and
returns -ENOMEM, leaving x->mode_data pointing at freed memory.

The xfrm clone unwind later runs destroy_state() through x->mode_data,
so the failed clone path tears down IPTFS state that clone_state()
already freed.

Keep the cloned IPTFS state private until all allocations succeed so
failed clones leave x->mode_data unset. The destroy path already
handles a NULL mode_data pointer.

Fixes: 4b3faf610cc63 ("xfrm: iptfs: add new iptfs xfrm mode impl")
Cc: stable@vger.kernel.org
Signed-off-by: Paul Moses <p@1g4.org>
---
 net/xfrm/xfrm_iptfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_iptfs.c b/net/xfrm/xfrm_iptfs.c
index 050a82101ca51..4d7a925f59b7c 100644
--- a/net/xfrm/xfrm_iptfs.c
+++ b/net/xfrm/xfrm_iptfs.c
@@ -2653,9 +2653,6 @@ static int iptfs_clone_state(struct xfrm_state *x, struct xfrm_state *orig)
 	if (!xtfs)
 		return -ENOMEM;
 
-	x->mode_data = xtfs;
-	xtfs->x = x;
-
 	xtfs->ra_newskb = NULL;
 	if (xtfs->cfg.reorder_win_size) {
 		xtfs->w_saved = kzalloc_objs(*xtfs->w_saved,
@@ -2666,6 +2663,9 @@ static int iptfs_clone_state(struct xfrm_state *x, struct xfrm_state *orig)
 		}
 	}
 
+	x->mode_data = xtfs;
+	xtfs->x = x;
+
 	return 0;
 }
 
-- 
2.53.GIT

Re: [net] xfrm: iptfs: only publish mode_data after clone setup

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
xfrm: iptfs: only publish mode_data after clone setup

iptfs_clone_state() stores x->mode_data before allocating the reorder
window. If that allocation fails, the code frees the cloned state and
returns -ENOMEM, leaving x->mode_data pointing at freed memory. The xfrm
clone unwind later runs destroy_state() through x->mode_data, so the failed
clone path tears down IPTFS state that clone_state() already freed. Keep
the cloned IPTFS state private until all allocations succeed so failed
clones leave x->mode_data unset.

> Fixes: 4b3faf610cc63 ("xfrm: iptfs: add new iptfs xfrm mode impl")

Should this Fixes tag point to 6be02e3e4f37 instead?

The original commit 4b3faf610cc63 added a simple iptfs_clone_state() function
that did not have the buggy allocation pattern. Commit 6be02e3e4f37 ("xfrm:
iptfs: handle reordering of received packets") added the reorder window
allocation code (xtfs->w_saved) to iptfs_clone_state(), but set x->mode_data
BEFORE the allocation, creating a use-after-free bug when the allocation
fails.

Suggested: Fixes: 6be02e3e4f37 ("xfrm: iptfs: handle reordering of received
packets")

Re: [net] xfrm: iptfs: only publish mode_data after clone setup

[Paul Moses]

> 
> > Fixes: 4b3faf610cc63 ("xfrm: iptfs: add new iptfs xfrm mode impl")
> 
> Should this Fixes tag point to 6be02e3e4f37 instead?
> 

Yes

Thanks,
Paul