Re: kern/60669: netipsec key_sp2msg buffer overrun

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

Great, thanks for the review, I will incorporate it into an updated
patch shortly!


> Date: Mon, 7 Sep 2026 12:16:19 +0900
> From: Kengo Nakahara <k-nakahara%iij.ad.jp@localhost>
> 
> # Add CC to ohishi who is IIJ IPsec co-worker
> 
> On 2026/09/01 1:26, Taylor R Campbell wrote:
> > Patch attached to fix and add tests for setsockopt/getsockopt2
> > IPV6_IPSEC_POLICY buffer overrun.  Review welcome!
> > 
> > Questions: Does it make sense to have mismatched src/dst address
> > families in a single IPsec policy? Does it make sense to have a
> > number of addresses in a policy other than zero or two?  If not, we
> > can block those various cases earlier and update the tests.
> 
> It does not make sense neither of them.  So, I think those cases
> can be blocked.
> 
> 
> > Should maybe do similarly for IPv4 but there's currently no getsockopt
> > or getsockopt2 path for it anyway -- it's under #if 0:
> > 
> >     1392 #if 0	/* defined(IPSEC) */
> >     1393 		case IP_IPSEC_POLICY:
> >     1394 		{
> >     1395 			struct mbuf *m = NULL;
> >     1396
> >     1397 			/* XXX this will return EINVAL as sopt is empty */
> >     1398 			error = ipsec_get_policy(inp, sopt->sopt_data,
> >     1399 			    sopt->sopt_size, &m);
> >     1400 			if (error == 0)
> >     1401 				error = sockopt_setmbuf(sopt, m);
> >     1402 			break;
> >     1403 		}
> >     1404 #endif /*IPSEC*/
> > 
> > https://nxr.NetBSD.org/xref/src/sys/netinet/ip_output.c?r=1.330#1392
> > 
> > The IPv6 version has the same comment but it is wrong because
> > getsockopt2 does allow the caller to pass input through sopt:
> > 
> >     1934 #if defined(IPSEC)
> >     1935 		case IPV6_IPSEC_POLICY:
> >     1936 			if (ipsec_used) {
> >     1937 				struct mbuf *m = NULL;
> >     1938
> >     1939 				/*
> >     1940 				 * XXX: this will return EINVAL as sopt is
> >     1941 				 * empty
> >     1942 				 */
> >     1943 				error = ipsec_get_policy(inp, sopt->sopt_data,
> >     1944 				    sopt->sopt_size, &m);
> >     1945 				if (!error)
> >     1946 					error = sockopt_setmbuf(sopt, m);
> >     1947 			} else
> >     1948 				error = ENOPROTOOPT;
> >     1949 			break;
> >     1950 #endif /* IPSEC */
> > 
> > https://nxr.NetBSD.org/xref/src/sys/netinet6/ip6_output.c?r=1.235#1934
> 
> I think that codes would be changed as they are old codes originated
> from FAST_IPSEC,
> 
> 
> At a glance of patch,
>      - It seems IS_PFKEY_ALIGNED (or more appropriate name macro)
>        should be introduced instead by (foo % 8) != 0 directly
>      - The following diff should be used KASSERTMSG instead of a comment
>        and KASSERT
> ====================
> @@ -2024,11 +2044,7 @@ static struct secpolicy *
>   		tlen -= xisr->sadb_x_ipsecrequest_len;
>   
>   		/* validity check */
> -		if (tlen < 0) {
> -			IPSECLOG(LOG_DEBUG, "becoming tlen < 0.\n");
> -			*error = EINVAL;
> -			goto free_exit;
> -		}
> +		KASSERT(tlen >= 0);
> ====================
> 
> The other parts look good to me.
> 
> 
> -- 
> //////////////////////////////////////////////////////////////////////
> Internet Initiative Japan Inc.
> 
> Device Engineering Section,
> Core Product Development Department,
> Network Division,
> Network Services Business Unit
> 
> Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
> 
> 
>