| Message ID | 20221027142455.3975224-1-chenzhihao@meizu.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xfrm:fix access to the null pointer in __xfrm_state_delete() | expand |
Hi Chen, You wrote on Thu, Oct 27, 2022 at 02:24:55PM +0000: > Validate the byseq node before removing it from the hlist of state_byseq. > km.seq cannot be used to determine whether the SA is in the byseq hlist > because xfrm_add_sa() may initialize km.seq to 0 and the SA is not inserted > into hlist. In later network communication, the seq field will increase > after the valid packet is received. > > In the above case, the NULL pointer will be accessed and cause a kernel > panic when the SA is being removed from hlist by checking km.seq field in > __xfrm_state_delete(). thanks for your patch! The solution is pretty close already, it's pfkey_send_new_mapping() from af_key.c messing with "x->km.seq" if a new NAT-T mapping is detected. I'll send a patch for the root cause on Monday, the commit message will be the most delicate thing. I already checked different PF_KEYv2 based IPSec implementations how they behave regarding SADB_X_NAT_T_NEW_MAPPING. Some details are public here already: https://github.com/strongswan/strongswan/issues/992#issuecomment-1294651331 If you are using a modern IPSec userspace application like strongswan 5.x, one can use the netlink xfrm interface and disable CONFIG_NET_KEY in the kernel to completely avoid the issue with PF_KEYv2. Also unloading the "af_key" kernel module helps. Perhaps it might make sense to extend your patch to WARN_ON in case we run into this situation again in the future? Then we would not sweep the issue under the rug. Cheers, Thomas
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index 3d2fe7712ac5..72a6426baef4 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -687,7 +687,7 @@ int __xfrm_state_delete(struct xfrm_state *x) list_del(&x->km.all); hlist_del_rcu(&x->bydst); hlist_del_rcu(&x->bysrc); - if (x->km.seq) + if (x->km.seq && !hlist_unhashed(&x->byseq)) hlist_del_rcu(&x->byseq); if (x->id.spi) hlist_del_rcu(&x->byspi);
Validate the byseq node before removing it from the hlist of state_byseq. km.seq cannot be used to determine whether the SA is in the byseq hlist because xfrm_add_sa() may initialize km.seq to 0 and the SA is not inserted into hlist. In later network communication, the seq field will increase after the valid packet is received. In the above case, the NULL pointer will be accessed and cause a kernel panic when the SA is being removed from hlist by checking km.seq field in __xfrm_state_delete(). Call trace: _xfrm_state_delete+0xb0/0x370 xfrm_del_sa+0x1c8/0x378 xfrm_user_rcv_msg+0x220/0x2d8 netlink_rcv_skb+0x104/0x180 xfrm_netlink_rcv+0x6c/0x118 netlink_unicast_kernel+0x12c/0x320 netlink_unicast+0x1dc/0x424 netlink_sendmsg+0x4b8/0x730 sock_write_iter+0x14c/0x1d8 do_iter_readv_writev+0x164/0x1d8 do_iter_write+0x104/0x2c4 do_writev+0x1a4/0x2d4 _arm64_sys_writev+0x24/0x34 invoke_syscall+0x60/0x150 el0svc_common+0xc8/0x114 do_el0_svc+0x28/0xa0 el0_svc+0x28/0x90 el0t_64_sync_handler+0x88/0xec el0t_64_sync+0x1b4/0x1b8 Fixes: fe9f1d8779cb ("xfrm: add state hashtable keyed by seq") Signed-off-by: Zhihao Chen <chenzhihao@meizu.com> Signed-off-by: Chonglong Xu <xuchonglong@meizu.com> --- net/xfrm/xfrm_state.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)