Re: [PATCH v3 net] net/packet: guard the ll header push in packet_rcv_spkt()

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (2) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The history and Fixes attribution in the commit message are
  inaccurate.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same CAN
  RX skb with mac_header == 0xFFFF (left that way by init_can_skb()…
- [High] This problem wasn't introduced by this patch, but the
  SOCK_PACKET receive path this patch touches leaks stale skb->cb bytes…

--

net/packet: guard the ll header push in packet_rcv_spkt()

This patch wraps the link layer header restore in packet_rcv_spkt() with
dev_has_header(), as packet_rcv() and tpacket_rcv() already do. The
motivation is a skb_under_panic() on CAN RX skbs whose mac_header is left
at the 0xFFFF sentinel, because init_can_skb() no longer resets the headers.

> commit d549699048b4 ("net/packet: fix packet receive on L3
> devices without visible hard header") introduced the helper and changed
> the two call sites, and this one stayed behind.

[Severity: Medium]
Is this history accurate?

In net/packet/af_packet.c, d549699048b4 changed

    if (dev->header_ops)

to

    if (dev_has_header(dev))

in packet_rcv() and tpacket_rcv(), and it also updated the block comment at
the top of the file. packet_rcv_spkt() never had a header_ops guard. Its
unconditional skb_push() goes back to the initial git import
(1da177e4c3f4).

The panic in the commit message only happens when mac_header is left at
0xFFFF. That has been the case since 9f10374bb024 ("can: remove private
CAN skb headroom infrastructure") removed the skb_reset_*_header() calls
from init_can_skb_reserve() and init_can_skb(). The current init_can_skb()
in drivers/net/can/dev/skb.c only does:

    skb->pkt_type = PACKET_BROADCAST;
    skb->ip_summed = CHECKSUM_UNNECESSARY;

> Fixes: d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header")
> Cc: stable@vger.kernel.org

With this Fixes tag and Cc: stable, the patch would be queued for the 5.10+
stable trees as a panic fix. CAN RX skbs in those trees still reset their
headers, so this panic cannot happen there.

9f10374bb024 is the commit that makes the panic reachable, and it isn't
named in any tag. Should the Fixes tag point at 9f10374bb024 instead?

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 7c83e01526ed..a951a1e3c088 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1911,7 +1911,8 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>  
>  	spkt = &PACKET_SKB_CB(skb)->sa.pkt;
>  
> -	skb_push(skb, skb->data - skb_mac_header(skb));
> +	if (dev_has_header(dev))
> +		skb_push(skb, skb->data - skb_mac_header(skb));

[Severity: High]
This isn't a bug introduced by this patch, but the same CAN RX skb with
mac_header == 0xFFFF also reaches generic XDP. This guard doesn't cover
that path.

netif_receive_generic_xdp() in net/core/dev.c does:

    mac_len = skb->data - skb_mac_header(skb);
    __skb_push(skb, mac_len);

Here mac_len comes out as roughly 4G - 65471, and __skb_push() only has a
DEBUG_NET warning. skb_headroom() then truncates to 65535, so no
reallocation happens, and the later __skb_pull() moves data back.

In bpf_prog_run_generic_xdp(), xdp_prepare_buff() gets a headroom of
skb_headroom - mac_len = 65535. That puts xdp->data at head + 65535, while
data_end is head + 80. The code then reads the Ethernet header at that
address:

    eth = (struct ethhdr *)xdp->data;
    orig_host = ether_addr_equal_64bits(eth->h_dest, skb->dev->dev_addr);

If a generic XDP program is attached to the CAN interface, can this read
out of bounds for every received CAN frame?

On XDP_TX or XDP_REDIRECT, __skb_push(skb, mac_len) also moves skb->data
about 4 GiB below head and sets a wrapped skb->len. The skb is then
transmitted or redirected.

dev_xdp_attach() has no device type restriction for generic mode. When a
program is attached, do_xdp_generic() runs for every RX skb:

  slcan RX
    alloc_can_skb()->init_can_skb()   /* mac_header stays 0xFFFF */
    netif_rx()
      __netif_receive_skb_core()
        do_xdp_generic()
          netif_receive_generic_xdp()
            bpf_prog_run_generic_xdp()

The af_packet guard alone doesn't close the 9f10374bb024 regression. Does
the separate init_can_skb() fix mentioned in the commit message need to
land together with this one?

>  
>  	/*
>  	 *	The SOCK_PACKET socket receives _all_ frames.

[Severity: High]
This is a pre-existing issue, but further down, packet_rcv_spkt() builds
the sockaddr_pkt in place in skb->cb:

    spkt->spkt_family = dev->type;
    strscpy(spkt->spkt_device, dev->name, sizeof(spkt->spkt_device));
    spkt->spkt_protocol = skb->protocol;

strscpy() writes strlen(name) + 1 bytes and doesn't pad, and nothing clears
the rest of spkt_device. packet_recvmsg() then copies the full
sizeof(struct sockaddr_pkt) to userspace:

    memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len);

Can this leak stale skb->cb bytes to userspace?

On GRO RX paths, cb still holds struct napi_gro_cb, and dev_gro_receive()
stores a slab pointer and jiffies there:

    NAPI_GRO_CB(skb)->age = jiffies;
    NAPI_GRO_CB(skb)->last = skb;

Nothing clears these before ptype_all delivery, and skb_share_check() and
skb_clone() copy cb.

With a 1 or 2 character interface name, cb[4..7] holds the upper 32 bits
of a kernel sk_buff address. With a 4 to 12 character name, jiffies still
leaks.

packet_create() checks CAP_NET_RAW with ns_capable(). A user in their own
user and network namespace can therefore create a veth with GRO enabled
and pick its name.

This dates back to the strlcpy() version. 8fc9d51ea2d32 swapped in
strscpy(), which doesn't pad either.

Would strscpy_pad(), or a memset() of the sockaddr_pkt before filling it,
be appropriate here?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260928113108.2127215-1-quchaosheng000406%40163.com