[PATCH] af_key: Fix heap information leak

[PATCH] af_key: Fix heap information leak

[Hyunwoo Kim]

Since x->calg etc. are allocated with kmalloc, information
in kernel heap can be leaked using netlink socket on
systems without CONFIG_INIT_ON_ALLOC_DEFAULT_ON.

Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
---
 net/key/af_key.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 2bdbcec781cd..9a7adcaf6aa3 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1174,7 +1174,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		}
 		if (key)
 			keysize = (key->sadb_key_bits + 7) / 8;
-		x->aalg = kmalloc(sizeof(*x->aalg) + keysize, GFP_KERNEL);
+		x->aalg = kzalloc(sizeof(*x->aalg) + keysize, GFP_KERNEL);
 		if (!x->aalg) {
 			err = -ENOMEM;
 			goto out;
@@ -1196,7 +1196,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 				err = -ENOSYS;
 				goto out;
 			}
-			x->calg = kmalloc(sizeof(*x->calg), GFP_KERNEL);
+			x->calg = kzalloc(sizeof(*x->calg), GFP_KERNEL);
 			if (!x->calg) {
 				err = -ENOMEM;
 				goto out;
@@ -1213,7 +1213,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 			key = (struct sadb_key*) ext_hdrs[SADB_EXT_KEY_ENCRYPT-1];
 			if (key)
 				keysize = (key->sadb_key_bits + 7) / 8;
-			x->ealg = kmalloc(sizeof(*x->ealg) + keysize, GFP_KERNEL);
+			x->ealg = kzalloc(sizeof(*x->ealg) + keysize, GFP_KERNEL);
 			if (!x->ealg) {
 				err = -ENOMEM;
 				goto out;
@@ -1261,7 +1261,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		const struct sadb_x_nat_t_type* n_type;
 		struct xfrm_encap_tmpl *natt;
 
-		x->encap = kmalloc(sizeof(*x->encap), GFP_KERNEL);
+		x->encap = kzalloc(sizeof(*x->encap), GFP_KERNEL);
 		if (!x->encap) {
 			err = -ENOMEM;
 			goto out;
-- 
2.25.1

Re: [PATCH] af_key: Fix heap information leak

[Herbert Xu]

On Sat, Feb 04, 2023 at 09:50:18AM -0800, Hyunwoo Kim wrote:
> Since x->calg etc. are allocated with kmalloc, information
> in kernel heap can be leaked using netlink socket on
> systems without CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
> 
> Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
> ---
>  net/key/af_key.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Please explain exactly what is leaked and how.

Thanks,
-- 
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

Re: [PATCH] af_key: Fix heap information leak

[Hyunwoo Kim]

On Tue, Feb 07, 2023 at 02:04:43PM +0800, Herbert Xu wrote:
> On Sat, Feb 04, 2023 at 09:50:18AM -0800, Hyunwoo Kim wrote:
> > Since x->calg etc. are allocated with kmalloc, information
> > in kernel heap can be leaked using netlink socket on
> > systems without CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
> > 
Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com

> > Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
> > ---
> >  net/key/af_key.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> Please explain exactly what is leaked and how.

ere's the KMSAN report reported by syzbot:
=====================================================
BUG: KMSAN: kernel-infoleak in instrument_copy_to_user include/linux/instrumented.h:121 [inline]
BUG: KMSAN: kernel-infoleak in copyout+0xbc/0x100 lib/iov_iter.c:169
 instrument_copy_to_user include/linux/instrumented.h:121 [inline]
 copyout+0xbc/0x100 lib/iov_iter.c:169
 _copy_to_iter+0x4f4/0x1fb0 lib/iov_iter.c:529
 copy_to_iter include/linux/uio.h:179 [inline]
 simple_copy_to_iter+0x64/0xa0 net/core/datagram.c:513
 __skb_datagram_iter+0x123/0xdc0 net/core/datagram.c:419
 skb_copy_datagram_iter+0x53/0x1d0 net/core/datagram.c:527
 skb_copy_datagram_msg include/linux/skbuff.h:3908 [inline]
 netlink_recvmsg+0x504/0x1650 net/netlink/af_netlink.c:1988
 ____sys_recvmsg+0x2c4/0x810
 ___sys_recvmsg+0x217/0x840 net/socket.c:2737
 do_recvmmsg+0x55a/0x1180 net/socket.c:2831
 __sys_recvmmsg net/socket.c:2910 [inline]
 __do_sys_recvmmsg net/socket.c:2933 [inline]
 __se_sys_recvmmsg net/socket.c:2926 [inline]
 __x64_sys_recvmmsg+0x3a7/0x4b0 net/socket.c:2926
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Uninit was stored to memory at:
 __nla_put lib/nlattr.c:1006 [inline]
 nla_put+0x1c2/0x230 lib/nlattr.c:1064
 copy_to_user_state_extra+0x115e/0x1aa0 net/xfrm/xfrm_user.c:1101
 dump_one_state+0x2c8/0x7c0 net/xfrm/xfrm_user.c:1169
 xfrm_state_walk+0x727/0x1300 net/xfrm/xfrm_state.c:2308
 xfrm_dump_sa+0x1e6/0x6b0 net/xfrm/xfrm_user.c:1240
 netlink_dump+0xb18/0x1560 net/netlink/af_netlink.c:2286
 __netlink_dump_start+0xa6d/0xc40 net/netlink/af_netlink.c:2391
 netlink_dump_start include/linux/netlink.h:294 [inline]
 xfrm_user_rcv_msg+0x828/0xf70 net/xfrm/xfrm_user.c:3091
 netlink_rcv_skb+0x3f1/0x750 net/netlink/af_netlink.c:2564
 xfrm_netlink_rcv+0x72/0xb0 net/xfrm/xfrm_user.c:3128
 netlink_unicast_kernel net/netlink/af_netlink.c:1330 [inline]
 netlink_unicast+0xf3b/0x1270 net/netlink/af_netlink.c:1356
 netlink_sendmsg+0x127d/0x1430 net/netlink/af_netlink.c:1932
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg net/socket.c:734 [inline]
 ____sys_sendmsg+0xa8e/0xe70 net/socket.c:2476
 ___sys_sendmsg+0x2a1/0x3f0 net/socket.c:2530
 __sys_sendmsg net/socket.c:2559 [inline]
 __do_sys_sendmsg net/socket.c:2568 [inline]
 __se_sys_sendmsg net/socket.c:2566 [inline]
 __x64_sys_sendmsg+0x367/0x540 net/socket.c:2566
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Uninit was created at:
 slab_post_alloc_hook mm/slab.h:766 [inline]
 slab_alloc_node mm/slub.c:3452 [inline]
 __kmem_cache_alloc_node+0x71f/0xce0 mm/slub.c:3491
 kmalloc_trace+0x4d/0x1f0 mm/slab_common.c:1062
 kmalloc include/linux/slab.h:580 [inline]
 pfkey_msg2xfrm_state net/key/af_key.c:1199 [inline]
 pfkey_add+0x3124/0x3b90 net/key/af_key.c:1504
 pfkey_process net/key/af_key.c:2844 [inline]
 pfkey_sendmsg+0x1693/0x1b90 net/key/af_key.c:3695
 sock_sendmsg_nosec net/socket.c:714 [inline]
 sock_sendmsg net/socket.c:734 [inline]
 ____sys_sendmsg+0xa8e/0xe70 net/socket.c:2476
 ___sys_sendmsg+0x2a1/0x3f0 net/socket.c:2530
 __sys_sendmmsg+0x40d/0xa40 net/socket.c:2616
 __do_sys_sendmmsg net/socket.c:2645 [inline]
 __se_sys_sendmmsg net/socket.c:2642 [inline]
 __x64_sys_sendmmsg+0xb8/0x120 net/socket.c:2642
 do_syscall_x64 arch/x86/entry/common.c:50 [inline]
 do_syscall_64+0x3d/0xb0 arch/x86/entry/common.c:80
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

Bytes 252-311 of 912 are uninitialized
Memory access of size 912 starts at ffff88811c76b000
Data copied to user address 0000000020001580

CPU: 1 PID: 5027 Comm: syz-executor292 Not tainted 6.2.0-rc3-syzkaller-79340-gc9a4e3bf8138 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/26/2022
=====================================================


After assigning `x->calg = kmalloc(sizeof(*x->calg), GFP_KERNEL);` in
pfkey_msg2xfrm_state(), only part of `x->calg->alg_name` is initialized,
so x->calg contains uninitialized kernel heap data.

Then open a netlink socket and use sendmsg() and recvmsg() to leak this
uninitialized kernel heap data to the user.

That is, the user can obtain the kernel memory address to bypass KASLR.

Of course, this can only be triggered on systems where
CONFIG_INIT_ON_ALLOC_DEFAULT_ON is not set.


Regards,
Hyunwoo Kim

> 
> Thanks,
> -- 
> 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

[PATCH] xfrm: Zero padding when dumping algos and encap

[Herbert Xu]

On Wed, Feb 08, 2023 at 12:54:34AM -0800, Hyunwoo Kim wrote:
> On Tue, Feb 07, 2023 at 02:04:43PM +0800, Herbert Xu wrote:
> > On Sat, Feb 04, 2023 at 09:50:18AM -0800, Hyunwoo Kim wrote:
> > > Since x->calg etc. are allocated with kmalloc, information
> > > in kernel heap can be leaked using netlink socket on
> > > systems without CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
> > > 
> Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com

Thanks.  This line should go into the patch description.

However, I don't think your patch is sufficient as xfrm_user
does the same thing as af_key.

I think a better approach would be to not copy out past the
end of string in copy_to_user_state_extra.  Something like
this:

---8<---
When copying data to user-space we should ensure that only valid
data is copied over.  Padding in structures may be filled with
random (possibly sensitve) data and should never be given directly
to user-space.

This patch fixes the copying of xfrm algorithms and the encap
template in xfrm_user so that padding is zeroed.

Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
Reported-by: Hyunwoo Kim <v4bel@theori.io>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index cf5172d4ce68..b5d50ae89840 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1012,7 +1012,9 @@ static int copy_to_user_aead(struct xfrm_algo_aead *aead, struct sk_buff *skb)
 		return -EMSGSIZE;
 
 	ap = nla_data(nla);
-	memcpy(ap, aead, sizeof(*aead));
+	strscpy_pad(ap->alg_name, aead->alg_name, sizeof(ap->alg_name));
+	ap->alg_key_len = aead->alg_key_len;
+	ap->alg_icv_len = aead->alg_icv_len;
 
 	if (redact_secret && aead->alg_key_len)
 		memset(ap->alg_key, 0, (aead->alg_key_len + 7) / 8);
@@ -1032,7 +1034,8 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
 		return -EMSGSIZE;
 
 	ap = nla_data(nla);
-	memcpy(ap, ealg, sizeof(*ealg));
+	strscpy_pad(ap->alg_name, ealg->alg_name, sizeof(ap->alg_name));
+	ap->alg_key_len = ealg->alg_key_len;
 
 	if (redact_secret && ealg->alg_key_len)
 		memset(ap->alg_key, 0, (ealg->alg_key_len + 7) / 8);
@@ -1043,6 +1046,40 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
 	return 0;
 }
 
+static int copy_to_user_calg(struct xfrm_algo *calg, struct sk_buff *skb)
+{
+	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*calg));
+	struct xfrm_algo *ap;
+
+	if (!nla)
+		return -EMSGSIZE;
+
+	ap = nla_data(nla);
+	strscpy_pad(ap->alg_name, calg->alg_name, sizeof(ap->alg_name));
+	ap->alg_key_len = 0;
+
+	return 0;
+}
+
+static int copy_to_user_encap(struct xfrm_encap_tmpl *ep, struct sk_buff *skb)
+{
+	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*ep));
+	struct xfrm_encap_tmpl *uep;
+
+	if (!nla)
+		return -EMSGSIZE;
+
+	uep = nla_data(nla);
+	memset(uep, 0, sizeof(*uep));
+
+	uep->encap_type = ep->encap_type;
+	uep->encap_sport = ep->encap_sport;
+	uep->encap_dport = ep->encap_dport;
+	uep->encap_oa = ep->encap_oa;
+
+	return 0;
+}
+
 static int xfrm_smark_put(struct sk_buff *skb, struct xfrm_mark *m)
 {
 	int ret = 0;
@@ -1098,12 +1135,12 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 			goto out;
 	}
 	if (x->calg) {
-		ret = nla_put(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
+		ret = copy_to_user_calg(x->calg, skb);
 		if (ret)
 			goto out;
 	}
 	if (x->encap) {
-		ret = nla_put(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
+		ret = copy_to_user_encap(x->encap, skb);
 		if (ret)
 			goto out;
 	}
-- 
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

Re: [PATCH] xfrm: Zero padding when dumping algos and encap

[Sabrina Dubroca]

2023-02-08, 18:24:03 +0800, Herbert Xu wrote:
> On Wed, Feb 08, 2023 at 12:54:34AM -0800, Hyunwoo Kim wrote:
> > On Tue, Feb 07, 2023 at 02:04:43PM +0800, Herbert Xu wrote:
> > > On Sat, Feb 04, 2023 at 09:50:18AM -0800, Hyunwoo Kim wrote:
> > > > Since x->calg etc. are allocated with kmalloc, information
> > > > in kernel heap can be leaked using netlink socket on
> > > > systems without CONFIG_INIT_ON_ALLOC_DEFAULT_ON.
> > > > 
> > Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
> 
> Thanks.  This line should go into the patch description.
> 
> However, I don't think your patch is sufficient as xfrm_user
> does the same thing as af_key.
> 
> I think a better approach would be to not copy out past the
> end of string in copy_to_user_state_extra.  Something like
> this:

Do you mean as a replacement for Hyunwoo's patch, or that both are
needed? pfkey_msg2xfrm_state doesn't always initialize encap_sport and
encap_dport (and calg->alg_key_len, but you're not using that in
copy_to_user_calg), so I guess you mean both patches.

> ---8<---
> When copying data to user-space we should ensure that only valid
> data is copied over.  Padding in structures may be filled with
> random (possibly sensitve) data and should never be given directly
> to user-space.
> 
> This patch fixes the copying of xfrm algorithms and the encap
> template in xfrm_user so that padding is zeroed.
> 
> Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
> Reported-by: Hyunwoo Kim <v4bel@theori.io>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index cf5172d4ce68..b5d50ae89840 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1012,7 +1012,9 @@ static int copy_to_user_aead(struct xfrm_algo_aead *aead, struct sk_buff *skb)
>  		return -EMSGSIZE;
>  
>  	ap = nla_data(nla);
> -	memcpy(ap, aead, sizeof(*aead));
> +	strscpy_pad(ap->alg_name, aead->alg_name, sizeof(ap->alg_name));
> +	ap->alg_key_len = aead->alg_key_len;
> +	ap->alg_icv_len = aead->alg_icv_len;
>  
>  	if (redact_secret && aead->alg_key_len)
>  		memset(ap->alg_key, 0, (aead->alg_key_len + 7) / 8);
> @@ -1032,7 +1034,8 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
>  		return -EMSGSIZE;
>  
>  	ap = nla_data(nla);
> -	memcpy(ap, ealg, sizeof(*ealg));
> +	strscpy_pad(ap->alg_name, ealg->alg_name, sizeof(ap->alg_name));
> +	ap->alg_key_len = ealg->alg_key_len;
>  
>  	if (redact_secret && ealg->alg_key_len)
>  		memset(ap->alg_key, 0, (ealg->alg_key_len + 7) / 8);
> @@ -1043,6 +1046,40 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
>  	return 0;
>  }
>  
> +static int copy_to_user_calg(struct xfrm_algo *calg, struct sk_buff *skb)
> +{
> +	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*calg));
> +	struct xfrm_algo *ap;
> +
> +	if (!nla)
> +		return -EMSGSIZE;
> +
> +	ap = nla_data(nla);
> +	strscpy_pad(ap->alg_name, calg->alg_name, sizeof(ap->alg_name));
> +	ap->alg_key_len = 0;
> +
> +	return 0;
> +}
> +
> +static int copy_to_user_encap(struct xfrm_encap_tmpl *ep, struct sk_buff *skb)
> +{
> +	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*ep));

XFRMA_ENCAP

> +	struct xfrm_encap_tmpl *uep;
> +
> +	if (!nla)
> +		return -EMSGSIZE;
> +
> +	uep = nla_data(nla);
> +	memset(uep, 0, sizeof(*uep));
> +
> +	uep->encap_type = ep->encap_type;
> +	uep->encap_sport = ep->encap_sport;
> +	uep->encap_dport = ep->encap_dport;
> +	uep->encap_oa = ep->encap_oa;

Should that be a memcpy? At least that's how xfrm_user.c usually does
copies of xfrm_address_t.

> +
> +	return 0;
> +}
> +
>  static int xfrm_smark_put(struct sk_buff *skb, struct xfrm_mark *m)
>  {
>  	int ret = 0;
> @@ -1098,12 +1135,12 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
>  			goto out;
>  	}
>  	if (x->calg) {
> -		ret = nla_put(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
> +		ret = copy_to_user_calg(x->calg, skb);
>  		if (ret)
>  			goto out;
>  	}
>  	if (x->encap) {
> -		ret = nla_put(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
> +		ret = copy_to_user_encap(x->encap, skb);
>  		if (ret)
>  			goto out;
>  	}
> -- 
> 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
> 

-- 
Sabrina

Re: [PATCH] xfrm: Zero padding when dumping algos and encap

[Herbert Xu]

On Wed, Feb 08, 2023 at 02:15:47PM +0100, Sabrina Dubroca wrote:
>
> Do you mean as a replacement for Hyunwoo's patch, or that both are
> needed? pfkey_msg2xfrm_state doesn't always initialize encap_sport and
> encap_dport (and calg->alg_key_len, but you're not using that in
> copy_to_user_calg), so I guess you mean both patches.

It's meant to be a replacement but yes we should still zero x->encap
because that will leak out in other ways, e.g., on the wire.

Hyunwoo, could you please repost your patch just for x->encap?

> > +static int copy_to_user_encap(struct xfrm_encap_tmpl *ep, struct sk_buff *skb)
> > +{
> > +	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*ep));
> 
> XFRMA_ENCAP

Good catch.  I will repost the patch.

> > +	uep->encap_oa = ep->encap_oa;
> 
> Should that be a memcpy? At least that's how xfrm_user.c usually does
> copies of xfrm_address_t.

It doesn't really matter.

Thanks,
-- 
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

Re: [PATCH] xfrm: Zero padding when dumping algos and encap

[Hyunwoo Kim]

On Thu, Feb 09, 2023 at 07:08:57AM +0800, Herbert Xu wrote:
> On Wed, Feb 08, 2023 at 02:15:47PM +0100, Sabrina Dubroca wrote:
> >
> > Do you mean as a replacement for Hyunwoo's patch, or that both are
> > needed? pfkey_msg2xfrm_state doesn't always initialize encap_sport and
> > encap_dport (and calg->alg_key_len, but you're not using that in
> > copy_to_user_calg), so I guess you mean both patches.
> 
> It's meant to be a replacement but yes we should still zero x->encap
> because that will leak out in other ways, e.g., on the wire.
> 
> Hyunwoo, could you please repost your patch just for x->encap?

Can the x->encap patch do this?

I didn't add the syzbot hash as x->encap is not the flow reported by syzbot:
Subject: [PATCH] af_key: Fix heap information leak

Since x->encap of pfkey_msg2xfrm_state() is not
initialized to 0, kernel heap data can be leaked.

Fix with kzalloc() to prevent this.

Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
---
 net/key/af_key.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 2bdbcec781cd..a815f5ab4c49 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1261,7 +1261,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		const struct sadb_x_nat_t_type* n_type;
 		struct xfrm_encap_tmpl *natt;

-		x->encap = kmalloc(sizeof(*x->encap), GFP_KERNEL);
+		x->encap = kzalloc(sizeof(*x->encap), GFP_KERNEL);
 		if (!x->encap) {
 			err = -ENOMEM;
 			goto out;
> 
> > > +static int copy_to_user_encap(struct xfrm_encap_tmpl *ep, struct sk_buff *skb)
> > > +{
> > > +	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*ep));
> > 
> > XFRMA_ENCAP
> 
> Good catch.  I will repost the patch.
> 
> > > +	uep->encap_oa = ep->encap_oa;
> > 
> > Should that be a memcpy? At least that's how xfrm_user.c usually does
> > copies of xfrm_address_t.
> 
> It doesn't really matter.
> 
> Thanks,
> -- 
> 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

Re: [PATCH] xfrm: Zero padding when dumping algos and encap

[Herbert Xu]

On Wed, Feb 08, 2023 at 09:34:16PM -0800, Hyunwoo Kim wrote:
>
> Can the x->encap patch do this?

Yes I think it should be sufficient.

> Subject: [PATCH] af_key: Fix heap information leak
> 
> Since x->encap of pfkey_msg2xfrm_state() is not
> initialized to 0, kernel heap data can be leaked.
> 
> Fix with kzalloc() to prevent this.
> 
> Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
> ---
>  net/key/af_key.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
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

[v2 PATCH] xfrm: Zero padding when dumping algos and encap

[Herbert Xu]

v2 fixes the mistaken type of XFRMA_ALG_COMP for x->encap.

---8<---
When copying data to user-space we should ensure that only valid
data is copied over.  Padding in structures may be filled with
random (possibly sensitve) data and should never be given directly
to user-space.

This patch fixes the copying of xfrm algorithms and the encap
template in xfrm_user so that padding is zeroed.

Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
Reported-by: Hyunwoo Kim <v4bel@theori.io>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index cf5172d4ce68..103af2b3e986 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1012,7 +1012,9 @@ static int copy_to_user_aead(struct xfrm_algo_aead *aead, struct sk_buff *skb)
 		return -EMSGSIZE;
 
 	ap = nla_data(nla);
-	memcpy(ap, aead, sizeof(*aead));
+	strscpy_pad(ap->alg_name, aead->alg_name, sizeof(ap->alg_name));
+	ap->alg_key_len = aead->alg_key_len;
+	ap->alg_icv_len = aead->alg_icv_len;
 
 	if (redact_secret && aead->alg_key_len)
 		memset(ap->alg_key, 0, (aead->alg_key_len + 7) / 8);
@@ -1032,7 +1034,8 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
 		return -EMSGSIZE;
 
 	ap = nla_data(nla);
-	memcpy(ap, ealg, sizeof(*ealg));
+	strscpy_pad(ap->alg_name, ealg->alg_name, sizeof(ap->alg_name));
+	ap->alg_key_len = ealg->alg_key_len;
 
 	if (redact_secret && ealg->alg_key_len)
 		memset(ap->alg_key, 0, (ealg->alg_key_len + 7) / 8);
@@ -1043,6 +1046,40 @@ static int copy_to_user_ealg(struct xfrm_algo *ealg, struct sk_buff *skb)
 	return 0;
 }
 
+static int copy_to_user_calg(struct xfrm_algo *calg, struct sk_buff *skb)
+{
+	struct nlattr *nla = nla_reserve(skb, XFRMA_ALG_COMP, sizeof(*calg));
+	struct xfrm_algo *ap;
+
+	if (!nla)
+		return -EMSGSIZE;
+
+	ap = nla_data(nla);
+	strscpy_pad(ap->alg_name, calg->alg_name, sizeof(ap->alg_name));
+	ap->alg_key_len = 0;
+
+	return 0;
+}
+
+static int copy_to_user_encap(struct xfrm_encap_tmpl *ep, struct sk_buff *skb)
+{
+	struct nlattr *nla = nla_reserve(skb, XFRMA_ENCAP, sizeof(*ep));
+	struct xfrm_encap_tmpl *uep;
+
+	if (!nla)
+		return -EMSGSIZE;
+
+	uep = nla_data(nla);
+	memset(uep, 0, sizeof(*uep));
+
+	uep->encap_type = ep->encap_type;
+	uep->encap_sport = ep->encap_sport;
+	uep->encap_dport = ep->encap_dport;
+	uep->encap_oa = ep->encap_oa;
+
+	return 0;
+}
+
 static int xfrm_smark_put(struct sk_buff *skb, struct xfrm_mark *m)
 {
 	int ret = 0;
@@ -1098,12 +1135,12 @@ static int copy_to_user_state_extra(struct xfrm_state *x,
 			goto out;
 	}
 	if (x->calg) {
-		ret = nla_put(skb, XFRMA_ALG_COMP, sizeof(*(x->calg)), x->calg);
+		ret = copy_to_user_calg(x->calg, skb);
 		if (ret)
 			goto out;
 	}
 	if (x->encap) {
-		ret = nla_put(skb, XFRMA_ENCAP, sizeof(*x->encap), x->encap);
+		ret = copy_to_user_encap(x->encap, skb);
 		if (ret)
 			goto out;
 	}
-- 
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

Re: [v2 PATCH] xfrm: Zero padding when dumping algos and encap

[Sabrina Dubroca]

2023-02-09, 09:09:52 +0800, Herbert Xu wrote:
> v2 fixes the mistaken type of XFRMA_ALG_COMP for x->encap.
> 
> ---8<---
> When copying data to user-space we should ensure that only valid
> data is copied over.  Padding in structures may be filled with
> random (possibly sensitve) data and should never be given directly
> to user-space.
> 
> This patch fixes the copying of xfrm algorithms and the encap
> template in xfrm_user so that padding is zeroed.
> 
> Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
> Reported-by: Hyunwoo Kim <v4bel@theori.io>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks Herbert.
Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

-- 
Sabrina

Re: [v2 PATCH] xfrm: Zero padding when dumping algos and encap

[Steffen Klassert]

On Thu, Feb 09, 2023 at 03:02:11PM +0100, Sabrina Dubroca wrote:
> 2023-02-09, 09:09:52 +0800, Herbert Xu wrote:
> > v2 fixes the mistaken type of XFRMA_ALG_COMP for x->encap.
> > 
> > ---8<---
> > When copying data to user-space we should ensure that only valid
> > data is copied over.  Padding in structures may be filled with
> > random (possibly sensitve) data and should never be given directly
> > to user-space.
> > 
> > This patch fixes the copying of xfrm algorithms and the encap
> > template in xfrm_user so that padding is zeroed.
> > 
> > Reported-by: syzbot+fa5414772d5c445dac3c@syzkaller.appspotmail.com
> > Reported-by: Hyunwoo Kim <v4bel@theori.io>
> > Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> Thanks Herbert.
> Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Applied, thanks a lot everyone!

[PATCH v2] af_key: Fix heap information leak

[Hyunwoo Kim]

Since x->encap of pfkey_msg2xfrm_state() is not
initialized to 0, kernel heap data can be leaked.

Fix with kzalloc() to prevent this.

Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
---
 net/key/af_key.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/key/af_key.c b/net/key/af_key.c
index 2bdbcec781cd..a815f5ab4c49 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -1261,7 +1261,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
 		const struct sadb_x_nat_t_type* n_type;
 		struct xfrm_encap_tmpl *natt;
 
-		x->encap = kmalloc(sizeof(*x->encap), GFP_KERNEL);
+		x->encap = kzalloc(sizeof(*x->encap), GFP_KERNEL);
 		if (!x->encap) {
 			err = -ENOMEM;
 			goto out;
-- 
2.25.1

Re: [PATCH v2] af_key: Fix heap information leak

[Sabrina Dubroca]

2023-02-09, 01:16:48 -0800, Hyunwoo Kim wrote:
> Since x->encap of pfkey_msg2xfrm_state() is not
> initialized to 0, kernel heap data can be leaked.
> 
> Fix with kzalloc() to prevent this.
> 
> Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>

Thanks.

> ---
>  net/key/af_key.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 2bdbcec781cd..a815f5ab4c49 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -1261,7 +1261,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
>  		const struct sadb_x_nat_t_type* n_type;
>  		struct xfrm_encap_tmpl *natt;
>  
> -		x->encap = kmalloc(sizeof(*x->encap), GFP_KERNEL);
> +		x->encap = kzalloc(sizeof(*x->encap), GFP_KERNEL);
>  		if (!x->encap) {
>  			err = -ENOMEM;
>  			goto out;
> -- 
> 2.25.1
> 

-- 
Sabrina

Re: [PATCH v2] af_key: Fix heap information leak

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Thu, 9 Feb 2023 01:16:48 -0800 you wrote:
> Since x->encap of pfkey_msg2xfrm_state() is not
> initialized to 0, kernel heap data can be leaked.
> 
> Fix with kzalloc() to prevent this.
> 
> Signed-off-by: Hyunwoo Kim <v4bel@theori.io>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> [...]

Here is the summary with links:
  - [v2] af_key: Fix heap information leak
    https://git.kernel.org/netdev/net/c/2f4796518315

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html