Re: [PATCH net v5 2/7] um: vector: trim skbs before verifying the header

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] In vector_mmsg_rx() (arch/um/drivers/vector_kern.c),
  pskb_trim(skb, ...) now runs before verify_header(), and the skb is…
- [Low] The commit message presents the reorder only as groundwork for
  the later commit "net: resolve uncommon partial checksums at ingress".

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  touches these exact lines.
- [Medium] This problem wasn't introduced by this patch, but
  vector_legacy_rx() sets the receive header iovec to vp->header_size…
- [Low] This problem wasn't introduced by this patch, but
  vector_legacy_rx() leaks the skb that prep_skb() allocated when…

--

um: vector: trim skbs before verifying the header

This patch moves the pskb_trim() call in vector_legacy_rx() and
vector_mmsg_rx() so that it runs before verify_header(). It prepares for a
follow-up change, in which virtio_net_hdr_to_skb() will complete the
checksum in software and will need the skb trimmed to the received length.

> Trim first, so the import sees the packet as it was received.

[Severity: Low]
Should the commit message say that this reorder also changes current
validation behaviour, and not only prepare for the follow-up?

raw_verify_header() calls virtio_net_hdr_to_skb(). Today,
__virtio_net_hdr_to_skb() already checks lengths against skb->len and
skb_headlen() in several places:

  skb_partial_csum_set()
  pskb_may_pull(skb, p_off)
  the UDP_L4 bound against gso_size * UDP_MAX_SEGMENTS
  the skb->len - nh_off > gso_size test that decides whether gso_size
  and SKB_GSO_DODGY get set

Before this patch, those checks ran against the untrimmed prep_skb()
buffer (max_packet plus frags). A vnet header whose csum_start/csum_offset
pointed past the received data, but still inside the buffer, used to pass.
The later trim then left a CHECKSUM_PARTIAL skb whose checksum location
was beyond skb->len.

With the reorder, such headers are rejected and counted in
rx_encaps_errors. Could the changelog describe this? There is also no
Fixes: tag.

> diff --git a/arch/um/drivers/vector_kern.c b/arch/um/drivers/vector_kern.c
> index 8a70b3a625537..1afbef78df725 100644
> --- a/arch/um/drivers/vector_kern.c
> +++ b/arch/um/drivers/vector_kern.c
> @@ -865,6 +865,7 @@ static int vector_legacy_rx(struct vector_private *vp)
>  
>  	if (skb != NULL) {

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. Does
vector_legacy_rx() leak the skb from prep_skb() when uml_vector_recvmsg()
fails?

Just above this hunk:

	pkt_len = uml_vector_recvmsg(vp->fds->rx_fd, &hdr, 0);
	if (pkt_len < 0) {
		vp->in_error = true;
		return pkt_len;
	}

uml_vector_recvmsg() returns -errno for any readv() failure other than
EAGAIN. This path then returns without calling dev_kfree_skb_irq(skb).
Nothing else holds a reference, so the skb and its frags are lost.

>  		if (pkt_len > vp->header_size) {
> +			pskb_trim(skb, pkt_len - vp->rx_header_size);
>  			if (vp->header_size > 0) {
>  				header_check = vp->verify_header(
>  					vp->header_rxbuffer, skb, vp);

[Severity: Medium]
This isn't a bug introduced by this patch, but is the header iovec in
vector_legacy_rx() the right size for IPv4 GRE and raw IPv4 L2TPv3?

Earlier in vector_legacy_rx():

	if (vp->header_size > 0) {
		iov[0].iov_base = vp->header_rxbuffer;
		iov[0].iov_len = vp->header_size;
	}

build_gre_transport_data() sets:

	vp->rx_header_size = vp->header_size;
	if (!td->ipv6)
		vp->rx_header_size += sizeof(struct iphdr);

build_l2tpv3_transport_data() does the same when
(!td->ipv6) && (!td->udp).

gre_verify_header() skips sizeof(struct iphdr) into header_rxbuffer
before it compares. With vec=0, readv() only puts header_size bytes (the
start of the IPv4 header) into header_rxbuffer. The verifier then reads
uninitialized kmalloc memory, so nearly all packets get dropped.

For any packet that does pass, the rest of the IP and encapsulation
header stays at the front of the skb. The trim by rx_header_size then
removes 20 bytes of real payload from the tail.

[ ... ]

> @@ -981,6 +981,8 @@ static int vector_mmsg_rx(struct vector_private *vp, int budget)
>  	for (i = 0; i < packet_count; i++) {
>  		skb = (*skbuff_vector);
>  		if (mmsg_vector->msg_len > vp->header_size) {
> +			pskb_trim(skb,
> +				mmsg_vector->msg_len - vp->rx_header_size);

[Severity: High]
Can skb be NULL here?

If the GFP_ATOMIC alloc_skb_with_frags() in prep_skb() fails, the
skbuff_vector slot stays NULL, but the header iovec is still active:

	if (vp->header_size > 0)
		iov_index++;
	if (result == NULL) {
		iov[iov_index].iov_base = NULL;
		iov[iov_index].iov_len = 0;
		goto done;
	}
	...
done:
	msg->msg_iovlen = iov_index;

vector_net_open() sizes the rx queue header buffers with
vp->rx_header_size through create_queue(). For IPv4 GRE and non-UDP IPv4
L2TPv3, rx_header_size is header_size + sizeof(struct iphdr). A packet
that fills only the header iovec therefore gives
msg_len == rx_header_size > header_size.

That packet enters this branch and calls pskb_trim(NULL, 0), which
dereferences skb->len.

Before this patch, gre_verify_header() or l2tpv3_verify_header() ran
first. Neither touches the skb. A packet that failed them went to
dev_kfree_skb_irq(NULL), which returns early. Only packets with a valid
header reached the old pskb_trim() on a NULL skb.

Now a packet with the wrong GRE key or the wrong L2TPv3 session/cookie
can also hit the NULL dereference whenever an rx skb allocation has
failed. The GRE and L2TPv3 raw sockets are bound but not connected, so
any remote host can send such packets.

The else branch already checks skb != NULL. Is a similar check needed
before this pskb_trim()?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
the patch touches these lines. Is this guard strict enough for the trim
length?

The guard checks msg_len > vp->header_size, but the trim subtracts
vp->rx_header_size. Two configurations have rx_header_size larger than
header_size:

  IPv4 GRE: header_size 4 and rx_header_size 24 without a key, or 8 and
  28 with a key
  raw IPv4 L2TPv3

In these cases, a msg_len between header_size and rx_header_size makes
the difference negative. pskb_trim() takes it as a very large unsigned
int:

	return (len < skb->len) ? __pskb_trim(skb, len) : 0;

and returns 0 without trimming. recvmmsg() only wrote into the header
iovec, so the skb keeps its full prep_skb() length of unwritten
alloc_skb_with_frags() memory. That memory goes through eth_type_trans()
and napi_gro_receive() into the guest stack.

gre_verify_header() and l2tpv3_verify_header() also read bytes past
msg_len in the per-slot header buffer. Those bytes can still hold an
earlier packet's header.

For example, a keyed GRE packet with msg_len 24 supplies only the 4 GRE
flag bytes. The key at offsets 24..27 is left over from the last valid
packet, so the key check passes without the sender knowing the key.

When msg_len == rx_header_size, the skb is trimmed to 0 bytes and
eth_type_trans() parses uninitialized head data.

vector_legacy_rx() has the same mismatch:

	if (pkt_len > vp->header_size) {
		pskb_trim(skb, pkt_len - vp->rx_header_size);

Should both paths require the received length to be larger than
rx_header_size (plus ETH_HLEN) before trimming?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.v5.git.mst%40redhat.com