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 <[email protected]> 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/[email protected]?part=2
