netdev - Re: [PATCH] af_key: Fix panic in dump_ah

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y/6zJnNRXtpYyaSJ@gondor.apana.org.au>
Date:   Wed, 1 Mar 2023 10:06:30 +0800
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Aichun Li <liaichun@...wei.com>
Cc:     davem@...emloft.net, kuba@...nel.org, netdev@...r.kernel.org,
        steffen.klassert@...unet.com, yanan@...wei.com,
        zhongxuan2@...wei.com
Subject: Re: [PATCH] af_key: Fix panic in dump_ah_combs()

On Tue, Feb 28, 2023 at 10:01:26PM +0800, Aichun Li wrote:
> Because the exclamation mark (!) is removed, the return value calculated by
>  count_esp_combs is smaller than that before ealg->available is removed. And in
>  dump_esp_combs, the number of struct sadb_combs in skb_put is larger than
>  alloc, This result in a buffer overrun.
> 
> [ 658.159619] ------------[ cut here ]------------
> [ 658.159629] kernel BUG at net/core/skbuff.c:110!
> [ 658.159733] invalid opcode: 0000 [#1] SMP KASAN NOPTI
> [ 658.160047] CPU: 14 PID: 107946 Comm: kernel_BUG_in_w Kdump: loaded Tainted: G I 5.10.0-60.18.0.50.x86_64 #1

Can you reproduce this on a mainline kernel?
 
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 8bc7d3999..bf2859c37 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -2927,7 +2927,7 @@ static int count_esp_combs(const struct xfrm_tmpl *t)
>  		if (!ealg->pfkey_supported)
>  			continue;
>  
> -		if (!(ealg_tmpl_set(t, ealg)))
> +		if (!(ealg_tmpl_set(t, ealg) && ealg->available))
>  			continue;

This makes no sense.  You are making the result of
count_esp_combs smaller.  How can that prevent a buffer overrun
if it was too small to begin with?

PS we could remove those brackets though, that would be a good
clean-up patch.

-		if (!(ealg_tmpl_set(t, ealg)))
+		if (!ealg_tmpl_set(t, ealg))

Cheers,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt