Re: [PATCH net v4 1/1] ipmr: only copy pktinfo to cache reports

[Ido Schimmel <idosch@nvidia.com>]

On Thu, Sep 03, 2026 at 02:38:16PM +0800, Zhiling Zou wrote:
> ipmr_cache_report() builds short IGMP reports for mrouted from a packet
> that may be a synthetic RTM_GETROUTE query. That query skb stores the
> netlink requester portid in NETLINK_CB(), but the report is delivered to
> a raw IPv4 socket, whose receive path interprets skb->cb as IPCB().
> 
> Commit bb7403655b3c ("ipmr: support IP_PKTINFO on cache report IGMP
> msg") added IP_PKTINFO support by calling ipv4_pktinfo_prepare() on the
> original packet and then copying the entire 48-byte skb control block to
> the report skb. For synthetic route-query packets, this copies
> NETLINK_CB() bytes into IPCB() and lets a controlled portid corrupt
> IPCB(skb)->opt. With IP_RECVOPTS or IP_RETOPTS enabled, the raw socket
> receive path can then copy past the short report packet or overflow the
> stack option buffer.
> 
> Keep the IP_PKTINFO support, but copy only the pktinfo fields prepared
> by ipv4_pktinfo_prepare().
> 
> The WHOLEPKT/WRVIFWHOLE path is a related leak: skb_realloc_headroom()
> inherits the original IPCB, then a 20-byte header with ihl = 5 is
> prepended. Stale IPCB(skb)->opt offsets then make ip_cmsg_recv_opts()
> copy bytes from after that header as IP_RECVOPTS. Clear the inherited
> IP options when that header is prepended.
> 
> This changes the accidental IP_RECVOPTS/IP_RETOPTS behavior for these
> cache reports, but that behavior was only a side effect of copying or
> inheriting the entire control block and was never intended to work.
> 
> Fixes: bb7403655b3c ("ipmr: support IP_PKTINFO on cache report IGMP msg")

The bug in the IGMPMSG_WHOLEPKT/IGMPMSG_WRVIFWHOLE path is older:

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
> ---
> changes in v4:
> - Clear IPCB(skb)->opt when prepending the 20-byte WHOLEPKT/WRVIFWHOLE
>   header, so stale option offsets are not inherited from the original
>   packet.
> - v3 Link: https://lore.kernel.org/all/e93d0d2fa5725ddd06b20e3e4223ab68b93b48d1.1785719031.git.zhilinz@nebusec.ai/
> 
> changes in v3:
> - go back to the v1 scope and stop changing the common path
> - remove memset(), since alloc_skb() already clears the control block
> - v2 Link: https://lore.kernel.org/all/1b809975e5bd9c0a1dd6fdd1db534e701fe5a4b6.1785379072.git.zhilinz@nebusec.ai/
> 
> changes in v2:
> - reword the commit message to say the entire 48-byte skb control block is copied
> - mention that IP_RECVOPTS/IP_RETOPTS on the mrouted socket was never intended
> - move the pktinfo copy to the common path so both report branches are covered
> - v1 Link: https://lore.kernel.org/all/5bc7cd71c2d671b305f25497d88ae8a0aa663c08.1784894076.git.zhilinz@nebusec.ai/
> 
>  net/ipv4/ipmr.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index e5f2b1c6150d2..a4d82a1bfe80f 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -1058,6 +1058,7 @@ static int ipmr_cache_report(const struct mr_table *mrt,
>  			     struct sk_buff *pkt, vifi_t vifi, int assert)
>  {
>  	const int ihl = ip_hdrlen(pkt);
> +	struct in_pktinfo *info;
>  	struct sock *mroute_sk;
>  	struct igmphdr *igmp;
>  	struct igmpmsg *msg;
> @@ -1102,6 +1103,7 @@ static int ipmr_cache_report(const struct mr_table *mrt,
>  		ip_hdr(skb)->ihl = sizeof(struct iphdr) >> 2;
>  		ip_hdr(skb)->tot_len = htons(ntohs(ip_hdr(pkt)->tot_len) +
>  					     sizeof(struct iphdr));
> +		memset(&IPCB(skb)->opt, 0, sizeof(IPCB(skb)->opt));
>  	} else {
>  		/* Copy the IP header */
>  		skb_set_network_header(skb, skb->len);
> @@ -1113,7 +1115,9 @@ static int ipmr_cache_report(const struct mr_table *mrt,
>  		msg->im_vif = vifi;
>  		msg->im_vif_hi = vifi >> 8;
>  		ipv4_pktinfo_prepare(mroute_sk, pkt, false);

pkt can continue in the data path and ipv4_pktinfo_prepare() will
overwrite IPCB(pkt)->iif with 0 if PKTINFO is disabled on the mroute
socket.

That's why I asked to try the following diff [1]. It also makes sure
that the PKTINFO reported for IGMPMSG_WHOLEPKT/IGMPMSG_WRVIFWHOLE
asserts is prepared with ipv4_pktinfo_prepare(), like in the other path.

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 1d9a4ac14fce..cdc84eb05c79 100644
--- a/net/ipv4/ipmr.c
+++ b/net/ipv4/ipmr.c
@@ -1057,6 +1057,7 @@ static void ipmr_cache_resolve(struct net *net, struct mr_table *mrt,
 static int ipmr_cache_report(const struct mr_table *mrt,
 			     struct sk_buff *pkt, vifi_t vifi, int assert)
 {
+	unsigned char pkt_cb[sizeof(pkt->cb)];
 	const int ihl = ip_hdrlen(pkt);
 	struct sock *mroute_sk;
 	struct igmphdr *igmp;
@@ -1112,8 +1113,6 @@ static int ipmr_cache_report(const struct mr_table *mrt,
 		msg = (struct igmpmsg *)skb_network_header(skb);
 		msg->im_vif = vifi;
 		msg->im_vif_hi = vifi >> 8;
-		ipv4_pktinfo_prepare(mroute_sk, pkt, false);
-		memcpy(skb->cb, pkt->cb, sizeof(skb->cb));
 		/* Add our header.
 		 * Note that code, csum and group fields are cleared.
 		 */
@@ -1124,6 +1123,12 @@ static int ipmr_cache_report(const struct mr_table *mrt,
 		skb->transport_header = skb->network_header;
 	}
 
+	memcpy(pkt_cb, pkt->cb, sizeof(pkt_cb));
+	ipv4_pktinfo_prepare(mroute_sk, pkt, false);
+	memset(skb->cb, 0, sizeof(skb->cb));
+	*PKTINFO_SKB_CB(skb) = *PKTINFO_SKB_CB(pkt);
+	memcpy(pkt->cb, pkt_cb, sizeof(pkt_cb));
+
 	igmpmsg_netlink_event(mrt, skb);
 
 	/* Deliver to mrouted */

[1] https://lore.kernel.org/netdev/20260805081737.GA1284302@shredder/

> -		memcpy(skb->cb, pkt->cb, sizeof(skb->cb));
> +		info = PKTINFO_SKB_CB(skb);
> +		info->ipi_ifindex = PKTINFO_SKB_CB(pkt)->ipi_ifindex;
> +		info->ipi_spec_dst = PKTINFO_SKB_CB(pkt)->ipi_spec_dst;
>  		/* Add our header.
>  		 * Note that code, csum and group fields are cleared.
>  		 */
> -- 
> 2.43.0
>