Re: [PATCH net v4 3/4] netdevsim: support devmem RX and TX bindings

[sashiko-bot@kernel.org]

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 <almasrymina@google.com>

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/20261015163915.1166043-1-almasrymina@google.com?part=3