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

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

[Zhiling Zou]

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().

This changes the accidental IP_RECVOPTS/IP_RETOPTS behavior for these
short cache reports, but that behavior was only a side effect of
copying the entire control block and was never intended to work.

Fixes: bb7403655b3c ("ipmr: support IP_PKTINFO on cache report IGMP msg")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Zhiling Zou <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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
index 1d9a4ac14fcef..56dfa43406c2d 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;
@@ -1113,7 +1114,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);
-		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

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

[Ido Schimmel]

On Mon, Aug 03, 2026 at 09:17:55AM +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().
> 
> This changes the accidental IP_RECVOPTS/IP_RETOPTS behavior for these
> short cache reports, but that behavior was only a side effect of
> copying the entire control block and was never intended to work.
> 
> Fixes: bb7403655b3c ("ipmr: support IP_PKTINFO on cache report IGMP msg")
> Cc: stable@vger.kernel.org
> Reported-by: Vega <vega@nebusec.ai>
> Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>

There are more bugs in ipmr_cache_report(). I will fix them in a
follow-up.

Reviewed-by: Ido Schimmel <idosch@nvidia.com>

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "Looks legit, let's fix both in one patch?"
---
ipmr: only copy pktinfo to cache reports

The patch stops ipmr_cache_report() from copying the whole 48-byte skb
control block from the original packet into the short IGMP cache report,
and instead copies only the two pktinfo fields prepared by
ipv4_pktinfo_prepare().  This keeps IP_PKTINFO working while preventing
non-IPCB cb bytes (for example NETLINK_CB of a synthetic RTM_GETROUTE
query) from being reinterpreted as IPCB() by the mrouted raw socket.

Link: https://lore.kernel.org/all/1b809975e5bd9c0a1dd6fdd1db534e701fe5a4b6.1785379072.git.zhilinz@nebusec.ai/
Link: https://lore.kernel.org/all/5bc7cd71c2d671b305f25497d88ae8a0aa663c08.1784894076.git.zhilinz@nebusec.ai/

> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 1d9a4ac14fce..56dfa43406c2 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c

[ ... ]

> @@ -1113,7 +1114,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);
> -		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;

[Severity: High]

This isn't a bug introduced by this patch, but does the same cb-versus-IPCB
mismatch remain on the other report branch of ipmr_cache_report()?

The branch fixed here is the alloc_skb(128) one, where the rest of the
control block stays zero.  The IGMPMSG_WHOLEPKT / IGMPMSG_WRVIFWHOLE branch
builds its report differently:

net/ipv4/ipmr.c:ipmr_cache_report() {
	if (assert == IGMPMSG_WHOLEPKT || assert == IGMPMSG_WRVIFWHOLE)
		skb = skb_realloc_headroom(pkt, sizeof(struct iphdr));
	...
		skb_push(skb, sizeof(struct iphdr));
		skb_reset_network_header(skb);
	...
		ip_hdr(skb)->ihl = sizeof(struct iphdr) >> 2;
}

skb_realloc_headroom() -> pskb_copy()/skb_clone() -> __copy_skb_header()
does:

net/core/skbuff.c:__copy_skb_header() {
	memcpy(new->cb, old->cb, sizeof(old->cb));
}

so the report inherits IPCB(pkt)->opt in full, including opt.optlen and the
option offsets opt.rr/ts/srr that ip_options_compile() stored as offsets
from the network header.  Both branches then end at the same
sock_queue_rcv_skb(mroute_sk, skb), and raw_recvmsg() -> ip_cmsg_recv()
reads that cb back as IPCB().

Since the branch pushes 20 bytes and rewrites ihl to 5, aren't those
inherited offsets now 20 bytes stale, pointing into the duplicated outer
header rather than at any options?

With IP_RETOPTS set on the mroute socket, can this smash the caller's stack
buffer?

net/ipv4/ip_sockglue.c:ip_cmsg_recv_retopts() {
	unsigned char optbuf[sizeof(struct ip_options) + 40];
	struct ip_options *opt = (struct ip_options *)optbuf;

	if (IPCB(skb)->opt.optlen == 0)
		return;

	if (ip_options_echo(net, opt, skb)) {
}

net/ipv4/ip_options.c:__ip_options_echo() {
		optlen  = sptr[sopt->rr+1];
		soffset = sptr[sopt->rr+2];
		dopt->rr = dopt->optlen + sizeof(struct iphdr);
		memcpy(dptr, sptr+sopt->rr, optlen);
}

The per-option length is re-read from the packet bytes at the stale offset
with no upper bound.  For sopt->rr == 24 (four IPOPT_NOOPs ahead of the RR
option) the length byte read is sptr[25], which after the push is byte 5 of
the original IP header, i.e. the low byte of the remote sender's IP ID.
Would that let up to 255 bytes be memcpy'd into the 40-byte option area of
optbuf[], with put_cmsg(..., opt->optlen, opt->__data) then handing the
result to user space?

The path looks reachable from a received multicast packet with ihl > 5:

ip_rcv_finish_core()      /* ip_rcv_options() fills IPCB(skb)->opt */
  dst_input()
    ip_mr_input()
      ip_mr_forward()
        ipmr_cache_report(mrt, skb, true_vifi, IGMPMSG_WRVIFWHOLE)

and via ipmr_prepare_xmit()/reg_vif_xmit() for IGMPMSG_WHOLEPKT.

For IP_RECVOPTS, ip_cmsg_recv_opts() copies IPCB(skb)->opt.optlen bytes
from ip_hdr(skb) + 1, which on this branch is the inner original header
instead of its options - are those cmsg contents also wrong here?

The changelog says:

    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.

and

    This changes the accidental IP_RECVOPTS/IP_RETOPTS behavior for these
    short cache reports, but that behavior was only a side effect of
    copying the entire control block and was never intended to work.

Given that the WHOLEPKT/WRVIFWHOLE branch has inherited the full cb via
skb_realloc_headroom() since well before bb7403655b3c, could the changelog
say that the second report path is deliberately left alone?  v2 covered
both branches, and v3 notes only "go back to the v1 scope and stop changing
the common path" without stating why the other branch is safe.

One smaller inconsistency: the WHOLEPKT/WRVIFWHOLE branch never calls
ipv4_pktinfo_prepare(), so for those reports PKTINFO_SKB_CB(skb)->ipi_ifindex
and ipi_spec_dst are really the inherited IPCB(pkt)->iif and
IPCB(pkt)->opt.faddr.  Is it intended that the two peer branches produce the
socket-visible IP_PKTINFO values by two different means?

>  		/* Add our header.
>  		 * Note that code, csum and group fields are cleared.
>  		 */
-- 
pw-bot: cr

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

[Ido Schimmel]

On Tue, Aug 04, 2026 at 06:31:26PM -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email says "Looks legit, let's fix both in one patch?"

I suggested [1], but the bug in the IGMPMSG_WHOLEPKT /
IGMPMSG_WRVIFWHOLE branch needs a different Fixes tag (it precedes
bb7403655b3c) and Sashiko [2][3] found another bug in this code:

ipv4_pktinfo_prepare() modifies the control block of 'pkt', which can
result in this packet being processed incorrectly after
ipmr_cache_report() returns. I *think* that [4] takes care of it, but
all of this needs to be explained in the commit message which should
carry the appropriate Fixes tags (bb7403655b3c + 1da177e4c3f4).

Note that this upcall is not fast path, so the extra work shouldn't be a
problem.

Is this what you prefer?

[1] https://lore.kernel.org/all/20260729091227.GA1258429@shredder/
[2] https://sashiko.dev/#/patchset/1b809975e5bd9c0a1dd6fdd1db534e701fe5a4b6.1785379072.git.zhilinz%40nebusec.ai
[3] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/1b809975e5bd9c0a1dd6fdd1db534e701fe5a4b6.1785379072.git.zhilinz%40nebusec.ai
[4]
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 */

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

[Jakub Kicinski]

On Wed, 5 Aug 2026 11:17:37 +0300 Ido Schimmel wrote:
> On Tue, Aug 04, 2026 at 06:31:26PM -0700, Jakub Kicinski wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email says "Looks legit, let's fix both in one patch?"  
> 
> I suggested [1], but the bug in the IGMPMSG_WHOLEPKT /
> IGMPMSG_WRVIFWHOLE branch needs a different Fixes tag (it precedes
> bb7403655b3c) and Sashiko [2][3] found another bug in this code:
> 
> ipv4_pktinfo_prepare() modifies the control block of 'pkt', which can
> result in this packet being processed incorrectly after
> ipmr_cache_report() returns. I *think* that [4] takes care of it, but
> all of this needs to be explained in the commit message which should
> carry the appropriate Fixes tags (bb7403655b3c + 1da177e4c3f4).

TBH no strong preference on the geometry of the patches (could also 
be two fixes in a series I guess?)  It'd be great if we could start
grouping the sufficiently similar fixes a little bit more. Otherwise
it's increasingly hard to make sense of the patch queue for me, if
nothing else.

> Note that this upcall is not fast path, so the extra work shouldn't be a
> problem.
> 
> Is this what you prefer?

diff looks good at a glance..

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

[Ido Schimmel]

On Wed, Aug 05, 2026 at 04:36:24PM -0700, Jakub Kicinski wrote:
> On Wed, 5 Aug 2026 11:17:37 +0300 Ido Schimmel wrote:
> > On Tue, Aug 04, 2026 at 06:31:26PM -0700, Jakub Kicinski wrote:
> > > This is an AI-generated review of your patch. The human sending this
> > > email says "Looks legit, let's fix both in one patch?"  
> > 
> > I suggested [1], but the bug in the IGMPMSG_WHOLEPKT /
> > IGMPMSG_WRVIFWHOLE branch needs a different Fixes tag (it precedes
> > bb7403655b3c) and Sashiko [2][3] found another bug in this code:
> > 
> > ipv4_pktinfo_prepare() modifies the control block of 'pkt', which can
> > result in this packet being processed incorrectly after
> > ipmr_cache_report() returns. I *think* that [4] takes care of it, but
> > all of this needs to be explained in the commit message which should
> > carry the appropriate Fixes tags (bb7403655b3c + 1da177e4c3f4).
> 
> TBH no strong preference on the geometry of the patches (could also 
> be two fixes in a series I guess?)  It'd be great if we could start
> grouping the sufficiently similar fixes a little bit more. Otherwise
> it's increasingly hard to make sense of the patch queue for me, if
> nothing else.
> 
> > Note that this upcall is not fast path, so the extra work shouldn't be a
> > problem.
> > 
> > Is this what you prefer?
> 
> diff looks good at a glance..

So let's go with one patch and two Fixes tags assuming Zhiling can
confirm that this diff works well and solves the reported problems.

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

[Jakub Kicinski]

On Wed, 5 Aug 2026 11:17:37 +0300 Ido Schimmel wrote:
> On Tue, Aug 04, 2026 at 06:31:26PM -0700, Jakub Kicinski wrote:
> > This is an AI-generated review of your patch. The human sending this
> > email says "Looks legit, let's fix both in one patch?"  
> 
> I suggested [1], but the bug in the IGMPMSG_WHOLEPKT /
> IGMPMSG_WRVIFWHOLE branch needs a different Fixes tag (it precedes
> bb7403655b3c) and Sashiko [2][3] found another bug in this code:

Orthogonal but do you wait for Clashiko (netdev-ai) before posting 
the review? If you do I'll assume you already screened it going forward?

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

[Ido Schimmel]

On Wed, Aug 05, 2026 at 04:37:40PM -0700, Jakub Kicinski wrote:
> On Wed, 5 Aug 2026 11:17:37 +0300 Ido Schimmel wrote:
> > On Tue, Aug 04, 2026 at 06:31:26PM -0700, Jakub Kicinski wrote:
> > > This is an AI-generated review of your patch. The human sending this
> > > email says "Looks legit, let's fix both in one patch?"  
> > 
> > I suggested [1], but the bug in the IGMPMSG_WHOLEPKT /
> > IGMPMSG_WRVIFWHOLE branch needs a different Fixes tag (it precedes
> > bb7403655b3c) and Sashiko [2][3] found another bug in this code:
> 
> Orthogonal but do you wait for Clashiko (netdev-ai) before posting 
> the review? If you do I'll assume you already screened it going forward?

I did screen it this time which is why I left a comment before tagging
the patch. You can count on it going forward.