Re: [PATCH] net/packet: zero-pad spkt_device and preserve sa.origlen across MSG_PEEK

[netdev-bot+sashiko@kernel.org]

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

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

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] Hunk #1 (strscpy() -> strscpy_pad() in packet_rcv_spkt(),
  net/packet/af_packet.c:1921) is a genuine kernel-memory information…
- [Low] The commit bundles two independent defects with different bug
  classes, different code paths and different regression lineages: (a)…
- [Low] The commit message mis-states the user-visible symptom of fix #2.

--

net/packet: zero-pad spkt_device and preserve sa.origlen across MSG_PEEK

This changes packet_rcv_spkt() to use strscpy_pad() when filling
spkt->spkt_device, and moves the sll_family/sll_protocol assignments in
packet_recvmsg() from PACKET_SKB_CB(skb) into the destination
msg->msg_name buffer so that the aliased sa.origlen is not clobbered.

No Link: tags were present in the commit header.

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 50cae32ae269..15def2d16806 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -1918,7 +1918,7 @@ static int packet_rcv_spkt(struct sk_buff *skb, struct net_device *dev,
>  	 */
>  
>  	spkt->spkt_family = dev->type;
> -	strscpy(spkt->spkt_device, dev->name, sizeof(spkt->spkt_device));
> +	strscpy_pad(spkt->spkt_device, dev->name, sizeof(spkt->spkt_device));
>  	spkt->spkt_protocol = skb->protocol;

[Severity: Medium]
Is this hunk actually closing a leak of uninitialized kernel memory to
userspace rather than just padding a buffer?

spkt here is PACKET_SKB_CB(skb)->sa.pkt, i.e. it is built in place at
offset 0 of skb->cb[48], which is never zeroed and carries residue from
earlier layers (for example qdisc_skb_cb on the transmit capture path via
dev_queue_xmit_nit()).  strscpy() terminates but does not pad, so for a
short name like "eth0" the remaining 9 bytes of the 14-byte spkt_device
keep whatever was in skb->cb.

packet_recvmsg() then copies the whole struct out with no memset of the
destination on the SOCK_PACKET branch:

	if (sock->type == SOCK_PACKET) {
		__sockaddr_check_size(sizeof(struct sockaddr_pkt));
		msg->msg_namelen = sizeof(struct sockaddr_pkt);
		copy_len = msg->msg_namelen;
	}
	...
	memcpy(msg->msg_name, &PACKET_SKB_CB(skb)->sa, copy_len);

so those bytes end up in the source address returned by
recvmsg()/recvfrom().

The sibling accessor packet_getname_spkt() already does:

	memset(uaddr->sa_data, 0, sizeof(uaddr->sa_data));
	strscpy(uaddr->sa_data, dev->name, sizeof(uaddr->sa_data));

which suggests the padding is a deliberate defence in this file rather
than hygiene.  The commit message only says "so trailing bytes after
dev->name's NUL terminator in the 14-byte buffer are zeroed" - could it
name the user-visible disclosure and carry its own Fixes: tag so stable
triage does not treat the hunk as cleanup?

[Severity: Low]
Should this hunk be a separate patch?  The single tag in the header:

	Fixes: 2472d7613bd3 ("net: packet: use sockaddr_ll fields as storage for skb original length in recvmsg path")

only describes the packet_recvmsg() change.  Reading 2472d7613bd3, it
introduced the origlen/sockaddr_ll union and the recvmsg writes, and it
did not touch packet_rcv_spkt() or this strscpy() call at all.

The two changes affect different socket types (SOCK_PACKET here,
SOCK_RAW/SOCK_DGRAM in packet_recvmsg()) and have different lineages, so
as submitted neither can be backported or reverted on its own.  Would
splitting this into two patches, each with its own Fixes: tag, work
better?

> @@ -3510,15 +3510,8 @@ static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  	if (err)
>  		goto out_free;
>  
> -	if (sock->type != SOCK_PACKET) {
> -		struct sockaddr_ll *sll = &PACKET_SKB_CB(skb)->sa.ll;
> -
> -		/* Original length was stored in sockaddr_ll fields */
> +	if (sock->type != SOCK_PACKET)
>  		origlen = PACKET_SKB_CB(skb)->sa.origlen;
> -		sll->sll_family = AF_PACKET;
> -		sll->sll_protocol = (sock->type == SOCK_DGRAM) ?
> -			vlan_get_protocol_dgram(skb) : skb->protocol;
> -	}

[Severity: Low]
Is the symptom described in the commit message the one this hunk fixes?
The message says the clobbered value affects:

	subsequent recvmsg(MSG_PEEK | MSG_TRUNC) calls
	on the same skb

which reads as though the length reported for MSG_TRUNC is wrong.  The
return value of packet_recvmsg() does not use origlen:

	err = vnet_hdr_len + ((flags&MSG_TRUNC) ? skb->len : copied);

The only consumer in net/packet/ is inside the PACKET_SOCK_AUXDATA block:

	if (packet_sock_flag(pkt_sk(sk), PACKET_SOCK_AUXDATA)) {
		...
		aux.tp_len = origlen;

So the observable appears to be a garbage tp_len in the PACKET_AUXDATA
cmsg on a second recvmsg() of a peeked skb (the old code had written
AF_PACKET | protocol << 16 over the aliased bytes), and only when
setsockopt(PACKET_AUXDATA) is enabled.  Could the message mention that
precondition, so that someone reproducing with MSG_PEEK|MSG_TRUNC alone
does not conclude the bug is not real?

>  	sock_recv_cmsgs(msg, sk, skb);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919215237.3470987-1-benquike%40gmail.com