Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] netdevsim: NULL pointer dereference in nsim_pp_hold_write() for VFs - [High] net: OOB access in nsim_do_psp() due to missing pskb_may_pull() - [Medium] bus: missing return value check for dma_coerce_mask_and_coherent() --
commit 4175c230c938d97450e1b621f172ccd40a24af4d Author: Mina Almasry <[email protected]> netdevsim: support devmem RX and TX bindings Set a 64-bit DMA mask on nsim_bus_dev, enable NETMEM_TX_DMA on the netdevsim net_device, and configure page_pool instances so devmem bind-rx and bind-tx netlink operations can be tested against netdevsim. > diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c > --- a/drivers/net/netdevsim/bus.c > +++ b/drivers/net/netdevsim/bus.c > @@ -462,6 +462,7 @@ nsim_bus_dev_new(unsigned int id, unsigned int > port_count, unsigned int num_queu > nsim_bus_dev->port_count = port_count; > nsim_bus_dev->num_queues = num_queues; > nsim_bus_dev->initial_net = current->nsproxy->net_ns; > + dma_coerce_mask_and_coherent(&nsim_bus_dev->dev, DMA_BIT_MASK(64)); [Severity: Medium] Does the return value of dma_coerce_mask_and_coherent() need to be checked here? If the platform rejects the requested 64-bit DMA mask, this function can return a negative error code. This might later cause page_pool_create() to fail or trigger silent DMA mapping issues when setting up the queues. > /* Disallow using nsim_bus_dev */ > smp_store_release(&nsim_bus_dev->init, false); [ ... ] > diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c > --- a/drivers/net/netdevsim/netdev.c > +++ b/drivers/net/netdevsim/netdev.c [ ... ] > @@ -929,12 +939,15 @@ nsim_pp_hold_write(struct file *file, const char __user > *data, > return ret; > > rtnl_lock(); > + netdev_lock(ns->netdev); > ret = count; > if (val == !!ns->page) > goto exit; > > if (!netif_running(ns->netdev) && val) { > ret = -ENETDOWN; > + } else if (val && page_pool_is_unreadable(ns->rq[0]->page_pool)) { [Severity: High] Could this lead to a NULL pointer dereference for VFs? When a VF is initialized, nsim_init_netdevsim_vf() is called instead of nsim_init_netdevsim(). This bypasses nsim_queue_init(), leaving ns->rq as NULL. Since the 'pp_hold' debugfs file is created unconditionally, writing '1' to it for a VF interface could reach this code and dereference ns->rq[0] while ns->rq itself is NULL. > + ret = -EOPNOTSUPP; > } else if (val) { > ns->page = page_pool_dev_alloc_pages(ns->rq[0]->page_pool); [ ... ] > diff --git a/drivers/net/netdevsim/psp.c b/drivers/net/netdevsim/psp.c > --- a/drivers/net/netdevsim/psp.c > +++ b/drivers/net/netdevsim/psp.c > @@ -86,6 +86,11 @@ nsim_do_psp(struct sk_buff *skb, struct netdevsim *ns, > __wsum csum; > int udplen; > > + if (!skb_frags_readable(skb)) { > + rc = SKB_DROP_REASON_PSP_OUTPUT; > + goto out_unlock; > + } > + > /* Do not decapsulate. Receive the skb with the udp and psp > * headers still there as if this is a normal udp packet. > * psp_dev_encapsulate() sets udp checksum to 0, so we need to > * provide a valid checksum here, so the skb isn't dropped. > */ > uh = udp_hdr(skb); > udplen = udp_get_len(skb, uh, skb_transport_offset(skb)); > csum = skb_checksum(skb, skb_transport_offset(skb), > udplen, 0); > > switch (skb->protocol) { > case htons(ETH_P_IP): > iph = ip_hdr(skb); [Severity: High] Is it safe to directly access udp_hdr(skb) and ip_hdr(skb) here without first verifying that the transport and network headers are in the linear data region? If the packet headers reside in paged fragments (for instance, if injected via AF_PACKET, since the netdevsim device advertises NETIF_F_SG), this might lead to an out-of-bounds memory access. Could a call to pskb_may_pull() be needed before accessing these header pointers? > uh->check = udp_v4_check(udplen, iph->saddr, > iph->daddr, csum); > break; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
