[PATCH] net: xfrm: fix bug in ipcomp_free_scratches

In ipcomp_alloc_scratches, if the vmalloc fails, there leaves a NULL
pointer. However, ipcomp_free_scratches does not check the per_pcu_ptr
pointer when invoking vfree.

Fix this by adding a sanity check before vfree.

Call Trace:
 ipcomp_free_scratches+0xbc/0x160 net/xfrm/xfrm_ipcomp.c:203
 ipcomp_free_data net/xfrm/xfrm_ipcomp.c:312 [inline]
 ipcomp_init_state+0x77c/0xa40 net/xfrm/xfrm_ipcomp.c:364
 ipcomp6_init_state+0xc2/0x700 net/ipv6/ipcomp6.c:154
 __xfrm_init_state+0x995/0x15c0 net/xfrm/xfrm_state.c:2648
 xfrm_init_state+0x1a/0x70 net/xfrm/xfrm_state.c:2675
 pfkey_msg2xfrm_state net/key/af_key.c:1287 [inline]
 pfkey_add+0x1a64/0x2cd0 net/key/af_key.c:1504
 pfkey_process+0x685/0x7e0 net/key/af_key.c:2837
 pfkey_sendmsg+0x43a/0x820 net/key/af_key.c:3676
 sock_sendmsg_nosec net/socket.c:703 [inline]
 sock_sendmsg+0xcf/0x120 net/socket.c:723

Reported-by: syzbot+b9cfd1cc5d57ee0a09ab@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Fixes: 6fccab671f2f ("ipsec: ipcomp - Merge IPComp impl")
Signed-off-by: Dongliang Mu <redacted>
---
 net/xfrm/xfrm_ipcomp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
index cb40ff0ff28d..9588ac05ab27 100644
--- a/net/xfrm/xfrm_ipcomp.c
+++ b/net/xfrm/xfrm_ipcomp.c

@@ -199,8 +199,11 @@ static void ipcomp_free_scratches(void)
 	if (!scratches)
 		return;
 
-	for_each_possible_cpu(i)
-		vfree(*per_cpu_ptr(scratches, i));
+	for_each_possible_cpu(i) {
+		void *scratch = *per_cpu_ptr(scratches, i);
+		if (!scratch)
+			vfree(scratch);
+	}
 
 	free_percpu(scratches);
 }

-- 
2.25.1

On Mon, Aug 16, 2021 at 03:38:29PM +0800, Dongliang Mu wrote:

-	for_each_possible_cpu(i)
-		vfree(*per_cpu_ptr(scratches, i));
+	for_each_possible_cpu(i) {
+		void *scratch = *per_cpu_ptr(scratches, i);
+		if (!scratch)
+			vfree(scratch);
+	}

This patch is unnecessary.  Please check the implementation of
vfree, it already checks for NULL pointers just like most of our
free primitives.

Cheers,
-- 
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Mon, Aug 16, 2021 at 3:53 PM Herbert Xu [off-list ref] wrote:

On Mon, Aug 16, 2021 at 03:38:29PM +0800, Dongliang Mu wrote:

quoted

-     for_each_possible_cpu(i)
-             vfree(*per_cpu_ptr(scratches, i));
+     for_each_possible_cpu(i) {
+             void *scratch = *per_cpu_ptr(scratches, i);
+             if (!scratch)
+                     vfree(scratch);
+     }

This patch is unnecessary.  Please check the implementation of
vfree, it already checks for NULL pointers just like most of our
free primitives.

Yes, you're right. Let me double-check the code and find out where the
problem is.

Cheers,
--
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Mon, Aug 16, 2021 at 3:53 PM Herbert Xu [off-list ref] wrote:

On Mon, Aug 16, 2021 at 03:38:29PM +0800, Dongliang Mu wrote:

quoted

-     for_each_possible_cpu(i)
-             vfree(*per_cpu_ptr(scratches, i));
+     for_each_possible_cpu(i) {
+             void *scratch = *per_cpu_ptr(scratches, i);
+             if (!scratch)
+                     vfree(scratch);
+     }

This patch is unnecessary.  Please check the implementation of
vfree, it already checks for NULL pointers just like most of our
free primitives.

Hi Herbert,

since there is no reproducer in the syzbot, I guess the problem might be:

if vmalloc_node in the ipcomp_alloc_scratches returns a NULL pointer,
it directly returns NULL without updating the per_cpu_ptr(scratches,
i).

Therefore, in the ipcomp_free_scratches, vfree will take an invalid
and outdated per_cpu_ptr as its argument, leading to the bug - BUG:
unable to handle kernel paging request in ipcomp_free_scratches.

Any idea?

Cheers,
--
Email: Herbert Xu [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt