Re: [PATCH net] ip: skip IPv4 cmsgs for non-IPv4 packets

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ip: skip IPv4 cmsgs for non-IPv4 packets

This adds an early return to ip_cmsg_recv_offset() when skb->protocol is
not ETH_P_IP, to avoid the IPv4 ancillary-data helpers parsing an
inet6_skb_parm as an inet_skb_parm after IPV6_ADDRFORM converts a socket
with an IPv6 skb still queued.

> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index a55ef327ec932..24cb74c3d6202 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c

[ ... ]

> @@ -171,6 +172,9 @@ void ip_cmsg_recv_offset(struct msghdr *msg, struct sock *sk,
>  {
>  	unsigned long flags = inet_cmsg_flags(inet_sk(sk));
>  
> +	if (unlikely(skb->protocol != htons(ETH_P_IP)))
> +		return;
> +

[Severity: High]
Does this drop IP_PKTINFO on the ipmr cache-report upcalls?

For IGMPMSG_NOCACHE and IGMPMSG_WRONGVIF, ipmr_cache_report() builds a
fresh skb and never runs it through ip_rcv()/eth_type_trans(), so
skb->protocol stays 0:

net/ipv4/ipmr.c:ipmr_cache_report() {
	...
	if (assert == IGMPMSG_WHOLEPKT || assert == IGMPMSG_WRVIFWHOLE)
		skb = skb_realloc_headroom(pkt, sizeof(struct iphdr));
	else
		skb = alloc_skb(128, GFP_ATOMIC);
	...
	} else {
		...
		ipv4_pktinfo_prepare(mroute_sk, pkt, false);
		memcpy(skb->cb, pkt->cb, sizeof(skb->cb));
	...
	/* Deliver to mrouted */
	ret = sock_queue_rcv_skb(mroute_sk, skb);
}

The ipv4_pktinfo_prepare()/memcpy() pair was added by commit bb7403655b3c3e
("ipmr: support IP_PKTINFO on cache report IGMP msg") specifically so that
mrouted/pimd get IP_PKTINFO for these upcalls, and the cb here is a valid
in_pktinfo.  The daemon reads it through raw_recvmsg():

net/ipv4/raw.c:raw_recvmsg() {
	...
	if (inet_cmsg_flags(inet))
		ip_cmsg_recv(msg, skb);
}

With the new guard, ip_cmsg_recv_offset() returns before
ip_cmsg_recv_pktinfo(), so the cmsg is gone for every multicast route miss.

Would this also make the two upcall flavours behave differently?  The
IGMPMSG_WHOLEPKT/IGMPMSG_WRVIFWHOLE variant goes through
skb_realloc_headroom(), which copies the headers group including protocol,
so it keeps ETH_P_IP and still gets its cmsgs, while the alloc_skb() variant
does not.

The second case is the tx-timestamp error queue.  ipv4_datagram_support_cmsg()
decides cmsg support from the cb and the socket tsflags, not from
skb->protocol, and pre-fills the pktinfo that ip_recv_error() then emits:

net/ipv4/ip_sockglue.c:ipv4_datagram_support_cmsg() {
	info = PKTINFO_SKB_CB(skb);
	if (!(READ_ONCE(sk->sk_tsflags) & SOF_TIMESTAMPING_OPT_CMSG) ||
	    !info->ipi_ifindex)
		return false;

	info->ipi_spec_dst.s_addr = ip_hdr(skb)->saddr;
	return true;
}

net/ipv4/ip_sockglue.c:ip_recv_error() {
	if (ipv4_datagram_support_cmsg(sk, skb, serr->ee.ee_origin)) {
		...
		if (inet_cmsg_flags(inet_sk(sk)))
			ip_cmsg_recv(msg, skb);
	}
}

For an IPv4 socket transmitting over a software-tagged VLAN device,
validate_xmit_skb() -> validate_xmit_vlan() -> __vlan_hwaccel_push_inside()
-> vlan_insert_tag_set_proto() sets skb->protocol = ETH_P_8021Q before the
driver calls skb_tx_timestamp().  The clone queued on the error queue then
has protocol ETH_P_8021Q, ipv4_datagram_support_cmsg() still returns true,
but the guard discards the IP_PKTINFO that SOF_TIMESTAMPING_OPT_CMSG is
meant to deliver for egress-device correlation.  The same holds for any path
that rewrites skb->protocol (for example encapsulation setting ETH_P_IPV6)
before the timestamp clone is taken.  The plain IPv4 tx path is unaffected
since ip_output() sets skb->protocol = htons(ETH_P_IP).

Would a discriminator tied more closely to the actual hazard avoid these
cases, for example rejecting only ETH_P_IPV6, gating only the cmsgs derived
from IPCB/the IPv4 header, or purging the mismatched skbs at IPV6_ADDRFORM
time?

The commit message says the change

    "preserves the queued payload while omitting incompatible ancillary data,
    and also protects callers processing a stale non-IPv4 error-queue skb."

Could this be extended to mention that intentionally supported cmsgs are
also omitted for genuine IPv4 deliveries, given the Cc: stable@vger.kernel.org
tag?
-- 
pw-bot: cr