Re: kern/60669: netipsec key_sp2msg buffer overrun

[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>