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

[Jakub Kicinski <kuba@kernel.org>]

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