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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] um: vector: NULL pointer dereference in vector_mmsg_rx() via pskb_trim()
--

commit 869705112c565c7836b0dd95f8fcf315b6c19398
Author: Michael S. Tsirkin <mst@redhat.com>

um: vector: trim skbs before verifying the header

This commit moves the pskb_trim() call in vector_legacy_rx() and
vector_mmsg_rx() ahead of vp->verify_header() so that the skb is trimmed to
the received packet length before header verification.

> 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

[ ... ]

> @@ -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 when pskb_trim() is called in vector_mmsg_rx() in
arch/um/drivers/vector_kern.c?

During vector_poll() -> vector_mmsg_rx() -> prep_queue_for_rx() -> prep_skb(),
if alloc_skb_with_frags() fails under memory pressure, prep_skb() leaves the
header iov[0] active with msg->msg_iovlen = 1 and stores a null skb pointer in
skbuff_vector:

arch/um/drivers/vector_kern.c:prep_skb() {
	...
	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;
	return result;
}

On IPv4 gre or IPv4 raw l2tpv3 interfaces, build_gre_transport_data() and
build_l2tpv3_transport_data() add sizeof(struct iphdr) to vp->rx_header_size,
and vector_net_open() initializes the rx queue with iov[0].iov_len set to
vp->rx_header_size.

When uml_vector_recvmmsg() receives a packet larger than vp->header_size into
iov[0] for a slot where skb allocation failed, mmsg_vector->msg_len is greater
than vp->header_size while skb is null:

arch/um/drivers/vector_kern.c:vector_mmsg_rx() {
	...
		} else {
			/* Overlay header too short to do anything - discard.
			 * We can actually keep this skb and reuse it,
			 * but that will make the prep logic too complex.
			 */
			if (skb != NULL)
				dev_kfree_skb_irq(skb);
		}
}

Previously, invalid tunnel packets that failed vp->verify_header() were
discarded via dev_kfree_skb_irq(skb) without dereferencing skb. With
pskb_trim(skb, ...) moved before vp->verify_header(), wouldn't this
immediately dereference a null skb, and shouldn't vector_mmsg_rx() check for a
null skb first like vector_legacy_rx() and the else branch do?

[ ... ]

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