Re: [dpdk-dev v4 2/2] net/tap: fix buffer overflow for ptypes list through driver API update

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 1/4/2024 5:51 PM, Sivaramakrishnan Venkat wrote:
> Incorrect ptypes list causes buffer overflow for Address Sanitizer
> run. Previously, the last element in the ptypes lists to be
> "RTE_PTYPE_UNKNOWN" for rte_eth_dev_get_supported_ptypes(), but this was
> not clearly documented and many PMDs did not follow this implementation.
> Instead, the dev_supported_ptypes_get() function pointer now returns the
> number of elements to eliminate the need for "RTE_PTYPE_UNKNOWN"
> as the last item.
> 
> Fixes: 47909357a069 ("ethdev: make device operations struct private")
> Cc: ferruh.yigit@intel.com
> Cc: stable@dpdk.org
> 

I think this is not fix, your previous patch fixes mentioned issue, but
this patch improved code to make sure it will be correct in the future.

Can you please update the commit log accordingly, remove fixes and
stable tag and patch title can be something like:
"drivers/net: return number of types in get supported ptypes"


> Signed-off-by: Sivaramakrishnan Venkat <venkatx.sivaramakrishnan@intel.com>
> ---
>  

<...>

> 
> diff --git a/drivers/net/atlantic/atl_ethdev.c b/drivers/net/atlantic/atl_ethdev.c
> index 3a028f4290..bc087738e4 100644
> --- a/drivers/net/atlantic/atl_ethdev.c
> +++ b/drivers/net/atlantic/atl_ethdev.c
> @@ -43,7 +43,8 @@ static int atl_dev_stats_reset(struct rte_eth_dev *dev);
>  static int atl_fw_version_get(struct rte_eth_dev *dev, char *fw_version,
>  			      size_t fw_size);
>  
> -static const uint32_t *atl_dev_supported_ptypes_get(struct rte_eth_dev *dev);
> +static const uint32_t *atl_dev_supported_ptypes_get(struct rte_eth_dev *dev,
> +	size_t *no_of_elements);
>  
>  static int atl_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu);
>  
> @@ -1132,7 +1133,8 @@ atl_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  }
>  
>  static const uint32_t *
> -atl_dev_supported_ptypes_get(struct rte_eth_dev *dev)
> +atl_dev_supported_ptypes_get(struct rte_eth_dev *dev,
> +	size_t *no_of_elements)
>

Can be single line, no need to break the line.
Same for some other drivers, can you please check all.

>  {
>  	static const uint32_t ptypes[] = {
>  		RTE_PTYPE_L2_ETHER,
> @@ -1143,12 +1145,13 @@ atl_dev_supported_ptypes_get(struct rte_eth_dev *dev)
>  		RTE_PTYPE_L4_TCP,
>  		RTE_PTYPE_L4_UDP,
>  		RTE_PTYPE_L4_SCTP,
> -		RTE_PTYPE_L4_ICMP,
> -		RTE_PTYPE_UNKNOWN
> +		RTE_PTYPE_L4_ICMP
>

Better to keep trailing ',' to reduce change when new ptypes appended in
the future, like:
"RTE_PTYPE_L4_ICMP,"

<...>

> @@ -348,7 +348,8 @@ dpaa_eth_dev_configure(struct rte_eth_dev *dev)
>  }
>  
>  static const uint32_t *
> -dpaa_supported_ptypes_get(struct rte_eth_dev *dev)
> +dpaa_supported_ptypes_get(struct rte_eth_dev *dev,
> +				size_t *no_of_elements)
>  {
>  	static const uint32_t ptypes[] = {
>  		RTE_PTYPE_L2_ETHER,
> @@ -363,14 +364,16 @@ dpaa_supported_ptypes_get(struct rte_eth_dev *dev)
>  		RTE_PTYPE_L4_TCP,
>  		RTE_PTYPE_L4_UDP,
>  		RTE_PTYPE_L4_SCTP,
> -		RTE_PTYPE_TUNNEL_ESP,
> -		RTE_PTYPE_UNKNOWN
> +		RTE_PTYPE_TUNNEL_ESP
>  	};
>  
>  	PMD_INIT_FUNC_TRACE();
>  
> -	if (dev->rx_pkt_burst == dpaa_eth_queue_rx)
> +	if (dev->rx_pkt_burst == dpaa_eth_queue_rx) {
> +		*no_of_elements = RTE_DIM(ptypes);
>  		return ptypes;
> +	}
> +	*no_of_elements = 0;


No need to set it to '0' when NULL returned, it is already 0 by default.
There are other drivers doing this, can you please check all?

<...>

> @@ -2261,19 +2261,22 @@ ice_dev_supported_ptypes_get(struct rte_eth_dev *dev)
>  		RTE_PTYPE_INNER_L4_UDP,
>  		RTE_PTYPE_TUNNEL_GTPC,
>  		RTE_PTYPE_TUNNEL_GTPU,
> -		RTE_PTYPE_L2_ETHER_PPPOE,
> -		RTE_PTYPE_UNKNOWN
> +		RTE_PTYPE_L2_ETHER_PPPOE
>  	};
>  
> -	if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS)
> +	if (ad->active_pkg_type == ICE_PKG_TYPE_COMMS) {
> +		*no_of_elements = RTE_DIM(ptypes_comms);
>  		ptypes = ptypes_comms;
> -	else
> +	} else {
> +		*no_of_elements = RTE_DIM(ptypes_os);
>  		ptypes = ptypes_os;
> +	}
>  
>  	if (dev->rx_pkt_burst == ice_recv_pkts ||
>  	    dev->rx_pkt_burst == ice_recv_pkts_bulk_alloc ||
> -	    dev->rx_pkt_burst == ice_recv_scattered_pkts)
> +	    dev->rx_pkt_burst == ice_recv_scattered_pkts) {
>  		return ptypes;
> +	}
>

No need to add { }, same below.

<...>


> @@ -3978,7 +3979,8 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>  }
>  
>  static const uint32_t *
> -ixgbe_dev_supported_ptypes_get(struct rte_eth_dev *dev)
> +ixgbe_dev_supported_ptypes_get(struct rte_eth_dev *dev,
> +		     size_t *no_of_elements)
>  {
>  	static const uint32_t ptypes[] = {
>  		/* For non-vec functions,
> @@ -3998,21 +4000,25 @@ ixgbe_dev_supported_ptypes_get(struct rte_eth_dev *dev)
>  		RTE_PTYPE_INNER_L3_IPV6,
>  		RTE_PTYPE_INNER_L3_IPV6_EXT,
>  		RTE_PTYPE_INNER_L4_TCP,
> -		RTE_PTYPE_INNER_L4_UDP,
> -		RTE_PTYPE_UNKNOWN
> +		RTE_PTYPE_INNER_L4_UDP
>  	};
>  
>  	if (dev->rx_pkt_burst == ixgbe_recv_pkts ||
>  	    dev->rx_pkt_burst == ixgbe_recv_pkts_lro_single_alloc ||
>  	    dev->rx_pkt_burst == ixgbe_recv_pkts_lro_bulk_alloc ||
> -	    dev->rx_pkt_burst == ixgbe_recv_pkts_bulk_alloc)
> +	    dev->rx_pkt_burst == ixgbe_recv_pkts_bulk_alloc) {
> +		*no_of_elements = RTE_DIM(ptypes);
>  		return ptypes;
> +	}
>  
>  #if defined(RTE_ARCH_X86) || defined(__ARM_NEON)
>  	if (dev->rx_pkt_burst == ixgbe_recv_pkts_vec ||
> -	    dev->rx_pkt_burst == ixgbe_recv_scattered_pkts_vec)
> +	    dev->rx_pkt_burst == ixgbe_recv_scattered_pkts_vec) {
> +		*no_of_elements = (sizeof(ptypes) / sizeof(uint32_t));
>

Why not use 'RTE_DIM()' here?

<...>


> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
> index b482cd12bb..9b03d27e62 100644
> --- a/lib/ethdev/ethdev_driver.h
> +++ b/lib/ethdev/ethdev_driver.h
> @@ -448,7 +448,8 @@ typedef int (*eth_dev_infos_get_t)(struct rte_eth_dev *dev,
>  				   struct rte_eth_dev_info *dev_info);
>  
>  /** @internal Get supported ptypes of an Ethernet device. */
> -typedef const uint32_t *(*eth_dev_supported_ptypes_get_t)(struct rte_eth_dev *dev);
> +typedef const uint32_t *(*eth_dev_supported_ptypes_get_t)(struct rte_eth_dev *dev,
> +				   size_t *no_of_elements);
>  

Can you please add doxygen comments, and document new paramter there.
There are samples in same file, like 'eth_promiscuous_enable_t' one.


Thanks,
Ferruh