[PATCH net] xfrm_user: fix info leak in build_mapping()

[PATCH net] xfrm_user: fix info leak in build_mapping()

[Greg Kroah-Hartman]

struct xfrm_usersa_id has a one-byte padding hole after the proto
field, which ends up never getting set to zero before copying out to
userspace.  Fix that up by zeroing out the whole structure before
setting individual variables.

Fixes: 3a2dfbe8acb1 ("xfrm: Notify changes in UDP encapsulation via netlink")
Cc: Steffen Klassert <steffen.klassert@secunet.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Simon Horman <horms@kernel.org>
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
Note, I think this is correct, as I don't think a new skb has it's
fields pre-zeroed out, or am I totally wrong here?

 net/xfrm/xfrm_user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8a854fa9567d..1bb8d05561df 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -4165,6 +4165,7 @@ static int build_mapping(struct sk_buff *skb, struct xfrm_state *x,
 
 	um = nlmsg_data(nlh);
 
+	memset(&um->id, 0, sizeof(um->id));
 	memcpy(&um->id.daddr, &x->id.daddr, sizeof(um->id.daddr));
 	um->id.spi = x->id.spi;
 	um->id.family = x->props.family;
-- 
2.53.0

Re: [PATCH net] xfrm_user: fix info leak in build_mapping()

[Jakub Kicinski]

On Mon,  6 Apr 2026 17:33:03 +0200 Greg Kroah-Hartman wrote:
> struct xfrm_usersa_id has a one-byte padding hole after the proto
> field, which ends up never getting set to zero before copying out to
> userspace.  Fix that up by zeroing out the whole structure before
> setting individual variables.
> 
> Fixes: 3a2dfbe8acb1 ("xfrm: Notify changes in UDP encapsulation via netlink")
> Cc: Steffen Klassert <steffen.klassert@secunet.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> Assisted-by: gregkh_clanker_t1000
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> Note, I think this is correct, as I don't think a new skb has it's
> fields pre-zeroed out, or am I totally wrong here?

You're right, skb owner is responsible for clearing after put.
Tho, Netlink is not as perf critical as real networking, I wish
we at least had a helper which reserves the space and clears it :/
This is not the first or the second time we hit this sort of a bug.

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 8a854fa9567d..1bb8d05561df 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -4165,6 +4165,7 @@ static int build_mapping(struct sk_buff *skb, struct xfrm_state *x,
>  
>  	um = nlmsg_data(nlh);
>  
> +	memset(&um->id, 0, sizeof(um->id));
>  	memcpy(&um->id.daddr, &x->id.daddr, sizeof(um->id.daddr));
>  	um->id.spi = x->id.spi;
>  	um->id.family = x->props.family;

Re: [PATCH net] xfrm_user: fix info leak in build_mapping()

[Jakub Kicinski]

On Mon, 6 Apr 2026 08:54:49 -0700 Jakub Kicinski wrote:
> > Note, I think this is correct, as I don't think a new skb has it's
> > fields pre-zeroed out, or am I totally wrong here?  
> 
> You're right, skb owner is responsible for clearing after put.
> Tho, Netlink is not as perf critical as real networking, I wish
> we at least had a helper which reserves the space and clears it :/
> This is not the first or the second time we hit this sort of a bug.

We could make nlmsg_append() do that. Mostly because I don't have 
a better idea for a name and nlmsg_append is only used once ;)

Re: [PATCH net] xfrm_user: fix info leak in build_mapping()

[Greg Kroah-Hartman]

On Mon, Apr 06, 2026 at 08:58:59AM -0700, Jakub Kicinski wrote:
> On Mon, 6 Apr 2026 08:54:49 -0700 Jakub Kicinski wrote:
> > > Note, I think this is correct, as I don't think a new skb has it's
> > > fields pre-zeroed out, or am I totally wrong here?  
> > 
> > You're right, skb owner is responsible for clearing after put.
> > Tho, Netlink is not as perf critical as real networking, I wish
> > we at least had a helper which reserves the space and clears it :/
> > This is not the first or the second time we hit this sort of a bug.
> 
> We could make nlmsg_append() do that. Mostly because I don't have 
> a better idea for a name and nlmsg_append is only used once ;)

As shown in my other patch:
	https://lore.kernel.org/r/2026040621-poison-gristle-aaa3@gregkh
we need this in at least 2 places, don't know if it's worth doing it for
all messages?

I guess nlmsg_append() would work?  It tries to do some zeroing out for
alignment for some reason...

Want me to do that?  I don't have a way to test any of this, I just
found it using some static code analysis tools that looked at holes in
structures.

thanks,

greg k-h

Re: [PATCH net] xfrm_user: fix info leak in build_mapping()

[Jakub Kicinski]

On Mon, 6 Apr 2026 18:08:27 +0200 Greg Kroah-Hartman wrote:
> On Mon, Apr 06, 2026 at 08:58:59AM -0700, Jakub Kicinski wrote:
> > On Mon, 6 Apr 2026 08:54:49 -0700 Jakub Kicinski wrote:  
> > > You're right, skb owner is responsible for clearing after put.
> > > Tho, Netlink is not as perf critical as real networking, I wish
> > > we at least had a helper which reserves the space and clears it :/
> > > This is not the first or the second time we hit this sort of a bug.  
> > 
> > We could make nlmsg_append() do that. Mostly because I don't have 
> > a better idea for a name and nlmsg_append is only used once ;)  
> 
> As shown in my other patch:
> 	https://lore.kernel.org/r/2026040621-poison-gristle-aaa3@gregkh
> we need this in at least 2 places, don't know if it's worth doing it for
> all messages?

I was thinking -- add the helper so that we can use it in places we're
touching anyway. No need to mess with correct existing code.

> I guess nlmsg_append() would work?  It tries to do some zeroing out for
> alignment for some reason...
> 
> Want me to do that?  I don't have a way to test any of this, I just
> found it using some static code analysis tools that looked at holes in
> structures.

Do you have any more Netlink leaks in the queue? If you do let's do it,
if you don't we can wait until the next victi^w patch to arrive.

Re: [PATCH net] xfrm_user: fix info leak in build_mapping()

[Greg Kroah-Hartman]

On Mon, Apr 06, 2026 at 10:38:51AM -0700, Jakub Kicinski wrote:
> On Mon, 6 Apr 2026 18:08:27 +0200 Greg Kroah-Hartman wrote:
> > On Mon, Apr 06, 2026 at 08:58:59AM -0700, Jakub Kicinski wrote:
> > > On Mon, 6 Apr 2026 08:54:49 -0700 Jakub Kicinski wrote:  
> > > > You're right, skb owner is responsible for clearing after put.
> > > > Tho, Netlink is not as perf critical as real networking, I wish
> > > > we at least had a helper which reserves the space and clears it :/
> > > > This is not the first or the second time we hit this sort of a bug.  
> > > 
> > > We could make nlmsg_append() do that. Mostly because I don't have 
> > > a better idea for a name and nlmsg_append is only used once ;)  
> > 
> > As shown in my other patch:
> > 	https://lore.kernel.org/r/2026040621-poison-gristle-aaa3@gregkh
> > we need this in at least 2 places, don't know if it's worth doing it for
> > all messages?
> 
> I was thinking -- add the helper so that we can use it in places we're
> touching anyway. No need to mess with correct existing code.
> 
> > I guess nlmsg_append() would work?  It tries to do some zeroing out for
> > alignment for some reason...
> > 
> > Want me to do that?  I don't have a way to test any of this, I just
> > found it using some static code analysis tools that looked at holes in
> > structures.
> 
> Do you have any more Netlink leaks in the queue? If you do let's do it,
> if you don't we can wait until the next victi^w patch to arrive.

I do not have any more, sorry.  So is it worth it for just these 2?
Your call :)