Re: [dpdk-dev] [PATCH 1/9] net/pcap: fix argument checks

["Ferriter, Cian" <cian.ferriter@intel.com>]

 

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Tuesday 1 October 2019 13:53
> To: dev@dpdk.org
> Cc: Kevin Traynor <ktraynor@redhat.com>; Ferriter, Cian
> <cian.ferriter@intel.com>; stable@dpdk.org
> Subject: [PATCH 1/9] net/pcap: fix argument checks
> 
> Previously rx/tx_queues were passed into eth_from_pcaps_common() as
> ptrs and were checked for being NULL.
> 
> In commit da6ba28f0540 ("net/pcap: use a struct to pass user options") that
> changed to pass in a ptr to a pmd_devargs_all which contains the
> rx/tx_queues.
> 
> The parameter checking was not updated as part of that commit and coverity
> caught that there was still a check if rx/tx_queues were NULL, apparently
> after they had been dereferenced.
> 
> Fix that by checking the pmd_devargs_all ptr and removing the NULL checks
> on rx/tx_queues.
> 
> 1231        struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
> 1232        struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
> 1233        const unsigned int nb_rx_queues = rx_queues->num_of_queue;
>     deref_ptr: Directly dereferencing pointer tx_queues.
> 1234        const unsigned int nb_tx_queues = tx_queues->num_of_queue;
> 1235        unsigned int i;
> 1236
> 1237        /* do some parameter checking */
>     CID 345004: Dereference before null check (REVERSE_INULL)
>     [select issue]
> 1238        if (rx_queues == NULL && nb_rx_queues > 0)
> 1239                return -1;
>     CID 345029 (#1 of 1): Dereference before null check (REVERSE_INULL)
>     check_after_deref: Null-checking tx_queues suggests that it may be
>     null, but it has already been dereferenced on all paths leading to
>     the check.
> 1240        if (tx_queues == NULL && nb_tx_queues > 0)
> 1241                return -1;
> 
> Coverity issue: 345029
> Coverity issue: 345044
> Fixes: da6ba28f0540 ("net/pcap: use a struct to pass user options")
> Cc: cian.ferriter@intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kevin Traynor <ktraynor@redhat.com>

Acked-by: Cian Ferriter <cian.ferriter@intel.com>


> ---
>  drivers/net/pcap/rte_eth_pcap.c | 19 +++++++++++--------
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c
> b/drivers/net/pcap/rte_eth_pcap.c index 5489010b6..7cf00306e 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -1229,16 +1229,19 @@ eth_from_pcaps_common(struct
> rte_vdev_device *vdev,  {
>  	struct pmd_process_private *pp;
> -	struct pmd_devargs *rx_queues = &devargs_all->rx_queues;
> -	struct pmd_devargs *tx_queues = &devargs_all->tx_queues;
> -	const unsigned int nb_rx_queues = rx_queues->num_of_queue;
> -	const unsigned int nb_tx_queues = tx_queues->num_of_queue;
> +	struct pmd_devargs *rx_queues;
> +	struct pmd_devargs *tx_queues;
> +	unsigned int nb_rx_queues;
> +	unsigned int nb_tx_queues;
>  	unsigned int i;
> 
> -	/* do some parameter checking */
> -	if (rx_queues == NULL && nb_rx_queues > 0)
> -		return -1;
> -	if (tx_queues == NULL && nb_tx_queues > 0)
> +	if (devargs_all == NULL)
>  		return -1;
> 
> +	rx_queues = &devargs_all->rx_queues;
> +	tx_queues = &devargs_all->tx_queues;
> +
> +	nb_rx_queues = rx_queues->num_of_queue;
> +	nb_tx_queues = tx_queues->num_of_queue;
> +
>  	if (pmd_init_internals(vdev, nb_rx_queues, nb_tx_queues,
> internals,
>  			eth_dev) < 0)
> --
> 2.21.0