Re: [PATCH net v3] amt: don't read the IP header from a reallocated skb head

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
says: "Sashiko points out more, let's fix them all cause the approach
of making a copy is fairy unusual, IDK if it will work for all headers."
---
amt: don't read the IP header from a reallocated skb head

This commit fixes several use-after-free bugs in AMT receive paths by
caching the source address before calling pskb_may_pull() or similar
helpers. It notes that pskb_may_pull() can reallocate the skb head,
making cached pointers like ip_hdr() dangling.

> The sibling handlers that re-derive ip_hdr() after the pull
> (amt_multicast_data_handler(), amt_membership_query_handler()) and
> the handlers that read the source address with no intervening pull
> (amt_discovery_handler(), amt_request_handler(), the IGMPv2/MLDv1
> report and leave handlers) are not affected.

[Severity: Critical]
This isn't a bug introduced by this patch, but the commit message states
that amt_multicast_data_handler() and amt_membership_query_handler() are
not affected.

Looking at amt_multicast_data_handler(), while ip_hdr() is re-derived,
the eth pointer is cached before calling pskb_may_pull():

amt_multicast_data_handler() {
    ...
    eth = eth_hdr(skb);

    if (!pskb_may_pull(skb, sizeof(*iph)))
        return true;

    iph = ip_hdr(skb);
    if (iph->version == 4) {
        ...
        eth->h_proto = htons(ETH_P_IP);
        ip_eth_mc_map(iph->daddr, eth->h_dest);
    }
}

Does this result in a use-after-free write if pskb_may_pull() reallocates
the skb head and frees the old buffer?

[Severity: Critical]
This isn't a bug introduced by this patch, but a similar issue exists in
amt_membership_query_handler(). The pointers amtmq, oeth, eth, and iph
are cached before calling pskb_may_pull().

If pskb_may_pull() reallocates the skb head, do these pointers become
dangling, causing use-after-free reads and writes when the function later
accesses amtmq->response_mac, iph->daddr, and modifies eth->h_proto?

[Severity: High]
This isn't a bug introduced by this patch, but in amt_multicast_data_handler(),
the code directly modifies the skb headroom by writing to eth->h_proto and
eth->h_dest.

Since the ethernet header overlays the pulled outer headers, this writes
directly into the skb's underlying data buffer. If the received skb is cloned
(for example, via tcpdump), does this corrupt the packet data seen by other
consumers since skb_unshare() or skb_cow_head() is not called?

> diff --git a/drivers/net/amt.c b/drivers/net/amt.c
> index 724a8163a5142..7094bdab0f463 100644
> --- a/drivers/net/amt.c
> +++ b/drivers/net/amt.c
> @@ -2000,13 +2000,15 @@ static void amt_igmpv3_report_handler(struct amt_dev *amt, struct sk_buff *skb,
>  	struct igmpv3_report *ihrv3 = igmpv3_report_hdr(skb);
>  	int len = skb_transport_offset(skb) + sizeof(*ihrv3);
>  	void *zero_grec = (void *)&igmpv3_zero_grec;
>  	struct amt_group_node *gnode;
>  	union amt_addr group, host;
>  	struct igmpv3_grec *grec;
> +	__be32 saddr;
>  	u16 nsrcs;
>  	int i;
>  
> +	saddr = ip_hdr(skb)->saddr;
> +
>  	for (i = 0; i < ntohs(ihrv3->ngrec); i++) {
>  		len += sizeof(*grec);
>  		if (!ip_mc_may_pull(skb, len))

[Severity: Critical]
This isn't a bug introduced by this patch, but since ip_mc_may_pull() can
reallocate the skb head, does evaluating ihrv3->ngrec in the loop condition
result in a use-after-free on the next iteration?

Additionally, grec is assigned inside the loop, followed immediately by
another ip_mc_may_pull() call. If this second pull reallocates the head,
does grec become dangling before grec->grec_mca and grec->grec_type are
accessed?

[ ... ]

> @@ -2162,13 +2164,15 @@ static void amt_mldv2_report_handler(struct amt_dev *amt, struct sk_buff *skb,
>  	struct mld2_report *mld2r = (struct mld2_report *)icmp6_hdr(skb);
>  	int len = skb_transport_offset(skb) + sizeof(*mld2r);
>  	void *zero_grec = (void *)&mldv2_zero_grec;
>  	struct amt_group_node *gnode;
>  	union amt_addr group, host;
>  	struct mld2_grec *grec;
> +	struct in6_addr saddr;
>  	u16 nsrcs;
>  	int i;
>  
> +	saddr = ipv6_hdr(skb)->saddr;
> +
>  	for (i = 0; i < ntohs(mld2r->mld2r_ngrec); i++) {
>  		len += sizeof(*grec);
>  		if (!ipv6_mc_may_pull(skb, len))

[Severity: Critical]
This isn't a bug introduced by this patch, but similarly, does evaluating
mld2r->mld2r_ngrec in this loop condition cause a use-after-free if
ipv6_mc_may_pull() reallocates the skb head?

[ ... ]

> @@ -2455,8 +2459,10 @@ static bool amt_update_handler(struct amt_dev *amt, struct sk_buff *skb)
>  	struct ethhdr *eth;
>  	struct iphdr *iph;
>  	int len, hdr_size;
> +	__be32 saddr;
>  
>  	iph = ip_hdr(skb);
> +	saddr = iph->saddr;
>  
>  	hdr_size = sizeof(*amtmu) + sizeof(struct udphdr);
>  	if (!pskb_may_pull(skb, hdr_size))

[Severity: Critical]
This isn't a bug introduced by this patch, but iph is cached here and used
later in the function to read iph->daddr:

amt_update_handler() {
    ...
    iph = ip_hdr(skb);
    if (iph->version == 4) {
        if (ip_mc_check_igmp(skb)) { ... }

        spin_lock_bh(&tunnel->lock);
        amt_igmp_report_handler(amt, skb, tunnel);
        spin_unlock_bh(&tunnel->lock);

        ...
        ip_eth_mc_map(iph->daddr, eth->h_dest);
    }
}

Both ip_mc_check_igmp() and amt_igmp_report_handler() can internally invoke
pskb_may_pull() or similar helpers. Does this cause a use-after-free read
of iph->daddr if the skb head is reallocated?

A similar pattern appears to exist in amt_dev_xmit() where iph->daddr is
read after ip_mc_check_igmp().

[Severity: High]
This isn't a bug introduced by this patch, but similar to the issue in
amt_multicast_data_handler(), amt_update_handler() directly modifies the
skb headroom:

amt_update_handler() {
    ...
    eth->h_proto = htons(ETH_P_IP);
    ip_eth_mc_map(iph->daddr, eth->h_dest);
}

Does this modify shared skb data without unsharing, potentially causing
silent data corruption if the skb is cloned?
-- 
pw-bot: cr