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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

netdev-bot+sashiko@ wrote:
> 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?

Agreed on all three points. Thanks for the fixes! Please do respin with

1. Separate the two fixes, with separate Fixes tags
2. For spkt_device describe that it solves a kernel mem leak through msg_name
3. For origlen, double check what the consequence of the peek overwrite is