Re: [PATCH net-next v2 3/7] um: vector: trim skbs before verifying the header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] net: vector: integer underflow in pskb_trim leads to info leak
- [High] um: vector: OOB read and packet corruption in vector_legacy_rx
--

commit 7f2cd5c3a1fb41cfaea6c03500daa85a7433a386
Author: Michael S. Tsirkin <mst@redhat.com>

um: vector: trim skbs before verifying the header

This commit modifies the vector receive path to trim the SKB before verifying
the transport header. This is done to prepare for computing the packet
checksum in software, which requires the packet to be trimmed correctly.

> 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) {
>  		if (pkt_len > vp->header_size) {
> +			pskb_trim(skb, pkt_len - vp->rx_header_size);

[Severity: High]
Can this subtraction underflow if pkt_len is smaller than vp->rx_header_size?

For GRE and L2TPv3 over IPv4, vp->rx_header_size (e.g., 24) is strictly
greater than vp->header_size (e.g., 4). If a packet is received with a
length between these two values (e.g., 20 bytes), the check
pkt_len > vp->header_size passes, but pkt_len - vp->rx_header_size
underflows to a very large unsigned value.

Because the length would exceed the pre-allocated skb->len, pskb_trim() fails
silently. The SKB retains its maximum allocated size containing uninitialized
memory.

If vp->verify_header() randomly succeeds on the undersized IOV buffer,
could this leak uninitialized kernel memory up the network stack via
napi_gro_receive()?

[Severity: High]
Does this sequence cause an out-of-bounds read and inner packet corruption?

Earlier in vector_legacy_rx(), iov[0].iov_len is restricted to
vp->header_size:

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

For raw IPv4 GRE/L2TPv3 tunnels where vp->header_size (4) differs from
vp->rx_header_size (24), recvmsg() reads only the first 4 bytes of the IP
header into vp->header_rxbuffer. The remaining IP header, GRE header, and
inner packet spill into iov[1] (skb->data).

When gre_verify_header() runs, it advances its pointer by
sizeof(struct iphdr) and reads uninitialized memory from the 4-byte
header_rxbuffer to verify the header.

Additionally, because the headers spilled into the SKB, does trimming by
the full vp->rx_header_size here incorrectly truncate the payload from
the tail instead of the head, silently destroying the inner packet
before it reaches eth_type_trans()?

[ ... ]

> @@ -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]
Like the vector_legacy_rx() path, does this subtraction also underflow if
msg_len is between vp->header_size and vp->rx_header_size?

>  			if (vp->header_size > 0) {
>  				header_check = vp->verify_header(
>  					mmsg_vector->msg_hdr.msg_iov->iov_base,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792067836.git.mst@redhat.com?part=3