Message-ID: <CADPKJ-5sUXaStr1TrNsgLF3vOq6+E3icGAkAh7xAWWWpGeep6g@mail.gmail.com>
Date: Fri, 7 Nov 2025 21:54:54 +0800
From: clingfei <clf700383@...il.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: horms@...nel.org, davem@...emloft.net, edumazet@...gle.com, 
	herbert@...dor.apana.org.au, kuba@...nel.org, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org, pabeni@...hat.com, steffen.klassert@...unet.com, 
	eadavis@...com, ssrane_b23@...vjti.ac.in, 
	syzbot+be97dd4da14ae88b6ba4@...kaller.appspotmail.com, 
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH 3/3] net: key: Validate address family in set_ipsecrequest()

Sabrina Dubroca <sd@...asysnail.net> 于2025年11月7日周五 01:17写道：
>
> note: There are a few issues with the format of this patch, and the
> subject prefix should be "[PATCH ipsec n/3]" for all the patches in
> the series. But I'm also not sure if this is the right way to fix this
> syzbot report.
>
>
> 2025-11-06, 21:56:58 +0800, clingfei wrote:
> > From: SHAURYA RANE <ssrane_b23@...vjti.ac.in>
>
>
> From here:
>
> > Hi syzbot,
> >
> > Please test the following patch.
> >
> > #syz test: git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
> >
> > Thanks,
> > Shaurya Rane
> >
> > From 123c5ac9ba261681b58a6217409c94722fde4249 Mon Sep 17 00:00:00 2001
> > From: Shaurya Rane <ssrane_b23@...vjti.ac.in>
> > Date: Sun, 19 Oct 2025 23:18:30 +0530
> > Subject: [PATCH] net: key: Validate address family in set_ipsecrequest()
>
> to here should be removed.
>

Sorry for the incorrect format of this patch. I will fix it in later patches.

>
> > syzbot reported a kernel BUG in set_ipsecrequest() due to an
> > skb_over_panic when processing XFRM_MSG_MIGRATE messages.
> >
> > The root cause is that set_ipsecrequest() does not validate the
> > address family parameter before using it to calculate buffer sizes.
> > When an unsupported family value (such as 0) is passed,
> > pfkey_sockaddr_len() returns 0, leading to incorrect size calculations.
> >
> > In pfkey_send_migrate(), the buffer size is calculated based on
> > pfkey_sockaddr_pair_size(), which uses pfkey_sockaddr_len(). When
> > family=0, this returns 0, so only sizeof(struct sadb_x_ipsecrequest)
> > (16 bytes) is allocated per entry. However, set_ipsecrequest() is
> > called multiple times in a loop (once for old_family, once for
> > new_family, for each migration bundle), repeatedly calling skb_put_zero()
> > with 16 bytes each time.
>
> So the root of the problem is a mismatch between allocation size and
> the actual size needed. Unexpected families are not good, sure, but
> would not cause a panic if the sizes were handled correctly.
>
> OTOH, for this old code which is being deprecated, maybe it doesn't
> matter to fix it "properly". (but see below)
>

I agree that the root cause of the problem is a mismatch between
 the allocation size and the actual size needed. I'm not familiar with the
kernel network stack, and I'm unsure if unexpected families might cause
other problems. But, regarding this specific issue, avoiding integer overflow
is sufficient to ensure consistency in size allocation and usage.

>
> > This causes the tail pointer to exceed the end pointer of the skb,
> > triggering skb_over_panic:
> >   tail: 0x188 (392 bytes)
> >   end:  0x180 (384 bytes)
> >
> > Fix this by validating that pfkey_sockaddr_len() returns a non-zero
> > value before proceeding with buffer operations. This ensures proper
> > size calculations and prevents buffer overflow. Checking socklen
> > instead of just family==0 provides comprehensive validation for all
> > unsupported address families.
> >
> > Reported-by: syzbot+be97dd4da14ae88b6ba4@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=be97dd4da14ae88b6ba4
> > Fixes: 08de61beab8a ("[PFKEYV2]: Extension for dynamic update of
> > endpoint address(es)")
> > Signed-off-by: Shaurya Rane <ssrane_b23@...vjti.ac.in>
> > ---
> >  net/key/af_key.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/key/af_key.c b/net/key/af_key.c
> > index cfda15a5aa4d..93c20a31e03d 100644
> > --- a/net/key/af_key.c
> > +++ b/net/key/af_key.c
> > @@ -3529,7 +3529,11 @@ static int set_ipsecrequest(struct sk_buff *skb,
> >       if (!family)
> >               return -EINVAL;
> >
> > -     size_req = sizeof(struct sadb_x_ipsecrequest) +
> > +    /* Reject invalid/unsupported address families */
>
> Steffen, AFAICT the whole migrate code has no family
> validation. Shouldn't we check {old,new}_family to be one of
> {AF_INET,AF_INET6} in xfrm_migrate_check? This should take care of the
> problems that this series tries to address, and avoid having objects
> installed in the kernel with unexpected families (which would match
> what validate_tmpl does).
>
>
> Looking quickly at xfrm_migrate_state_find, it also seems to compare
> addresses without checking that both addresses are of the same
> family. That seems a bit wrong, but changing the behavior of that old
> code is maybe too risky.
>
>
>
> > +    if (!socklen)
> > +        return -EINVAL;
> > +
> > +    size_req = sizeof(struct sadb_x_ipsecrequest) +
>
> nit: tabs should be used, not spaces
>
> >                  pfkey_sockaddr_pair_size(family);
> >
> >       rq = skb_put_zero(skb, size_req);
>
> --
> Sabrina

I think the check on socklen is trying to reject unexpected families,
but I am not sure if it is too late, and this check can only take
effect when the
type of family is handled successfully.

Powered by blists - more mailing lists