[PATCH net] net: af_key: fix use-after-free in dump_sp() on policy migrate

[PATCH net] net: af_key: fix use-after-free in dump_sp() on policy migrate

[Daniel Wade]

pfkey_xfrm_policy2msg_size() derives an exported policy's message length
from xfrm_vec[].encap_family. dump_sp() derives it twice: once through
pfkey_xfrm_policy2msg_prep(), which allocates the skb, and again in
pfkey_xfrm_policy2msg(), which fills it in.

dump_sp() holds net->xfrm.xfrm_policy_lock rather than pol->lock, so
xfrm_policy_migrate() can rewrite encap_family between the two
derivations. The second length is then up to 24 bytes per template
larger than the first, and pfkey_xfrm_policy2msg() opens with
memset(hdr, 0, size), clearing the difference into the skb's shared
info. Four BEET templates put 80 bytes there, which reaches dataref:
alloc_skb() had set it to 1, the memset makes it 0, and the skb_clone()
in pfkey_broadcast_one() leaves two skbs sharing one head with a dataref
of 1. Freeing the original releases a head the queued clone still points
at, so recvmsg() copies freed memory to user space and consume_skb()
frees it a second time. The CAP_NET_ADMIN that pfkey_create() and the
xfrm netlink receive path check is namespace-relative, so an
unprivileged user can reach this. SLUB's double-free check fires within
a second of starting the reproducer; the oops kills a task, or panics
the machine when the corruption lands in softirq context.

Racing SADB_X_SPDDUMP against XFRM_MSG_MIGRATE as an ordinary user gave
4585 use-after-free reports and 765 double-frees in 300s on mainline
with KASAN and 8 CPUs. The reads reach user space (addresses trimmed):

  BUG: KASAN: slab-use-after-free in _copy_to_iter+0x1d8/0x1430
  Read of size 240 at addr ffff88810f98d140 by task pfkey_dataref_o/433
  CPU: 3 UID: 65534 PID: 433 Comm: pfkey_dataref_o Not tainted 7.4.0-rc2
  Call Trace:
   <TASK>
   dump_stack_lvl+0x66/0xa0
   print_report+0xd0/0x600
   kasan_report+0xc6/0x100
   kasan_check_range+0x105/0x1b0
   _copy_to_iter+0x1d8/0x1430
   __skb_datagram_iter+0xf8/0x820
   skb_copy_datagram_iter+0x6c/0x1a0
   pfkey_recvmsg+0x1ff/0x500
   sock_recvmsg+0x15e/0x1a0
   __sys_recvfrom+0x182/0x260
   do_syscall_64+0x10b/0x5f0
   entry_SYSCALL_64_after_hwframe+0x77/0x7f
  Allocated by task 433:
   kmalloc_reserve+0x108/0x2b0
   __alloc_skb+0x123/0x5b0
   dump_sp+0xdf/0x490
   xfrm_policy_walk+0x1d9/0x6c0
   pfkey_do_dump+0x61/0x420
   pfkey_recvmsg+0x358/0x500

Take xp->lock for reading across both calls, so both derivations see the
same encap_family. xfrm_policy_walk() already calls dump_sp() under
spin_lock_bh(), so the plain reader suffices, as in xfrm_policy_timer().
The other callers of this pair are already serialized against migrate by
net->xfrm.xfrm_cfg_mutex.

Fixes: 80c9abaabf42 ("[XFRM]: Extension for dynamic update of endpoint address(es)")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Wade <danjwade95@gmail.com>
---

 net/key/af_key.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index d215b33..ab4aaca 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -2712,11 +2712,18 @@ static int dump_sp(struct xfrm_policy *xp, int dir, int count, void *ptr)
 	if (!pfkey_can_dump(&pfk->sk))
 		return -ENOBUFS;
 
+	/* Both halves derive the size from xfrm_vec[].encap_family, so
+	 * hold xp->lock against a concurrent xfrm_policy_migrate().
+	 */
+	read_lock(&xp->lock);
 	out_skb = pfkey_xfrm_policy2msg_prep(xp);
-	if (IS_ERR(out_skb))
+	if (IS_ERR(out_skb)) {
+		read_unlock(&xp->lock);
 		return PTR_ERR(out_skb);
+	}
 
 	err = pfkey_xfrm_policy2msg(out_skb, xp, dir);
+	read_unlock(&xp->lock);
 	if (err < 0) {
 		kfree_skb(out_skb);
 		return err;

Re: [PATCH net] net: af_key: fix use-after-free in dump_sp() on policy migrate

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account.

*IMPORTANT*: If you do repost (for other reasons) - please include
the relevant info in the cover letter or commit messages.
-- 
The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.