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 mudongliangabcd@gmail.com --- 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); }
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,
On Mon, Aug 16, 2021 at 3:53 PM Herbert Xu herbert@gondor.apana.org.au wrote:
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.
Yes, you're right. Let me double-check the code and find out where the problem is.
Cheers,
Email: Herbert Xu herbert@gondor.apana.org.au 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 herbert@gondor.apana.org.au wrote:
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.
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 herbert@gondor.apana.org.au Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
linux-stable-mirror@lists.linaro.org