Re: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet

[Dan Carpenter <dan.carpenter@oracle.com>]

On Fri, Nov 20, 2015 at 06:35:38PM -0500, James Simmons wrote:
> From: Sebastien Buisson <sebastien.buisson@bull.net>
> 
> Fix 'NULL pointer dereference' defects found by Coverity version
> 6.5.3:
> Dereference after null check (FORWARD_NULL)
> For instance, Passing null pointer to a function which dereferences
> it.
> Dereference before null check (REVERSE_INULL)
> Null-checking variable suggests that it may be null, but it has
> already been dereferenced on all paths leading to the check.
> Dereference null return value (NULL_RETURNS)
> 
> The following fixes for the LNet layer are broken out of patch
> http://review.whamcloud.com/4720.
> 
> Signed-off-by: Sebastien Buisson <sebastien.buisson@bull.net>
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2217
> Reviewed-on: http://review.whamcloud.com/4720
> Reviewed-by: Dmitry Eremin <dmitry.eremin@intel.com>
> Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
> ---
>  .../staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c    |    2 +-
>  drivers/staging/lustre/lnet/lnet/lib-move.c        |    2 +
>  drivers/staging/lustre/lnet/selftest/conctl.c      |   51 ++++++++++----------
>  3 files changed, 29 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> index de0f85f..0f4154c 100644
> --- a/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> +++ b/drivers/staging/lustre/lnet/klnds/o2iblnd/o2iblnd.c
> @@ -2829,7 +2829,7 @@ int kiblnd_startup(lnet_ni_t *ni)
>  	return 0;
>  
>  failed:
> -	if (net->ibn_dev == NULL && ibdev != NULL)
> +	if (net && net->ibn_dev == NULL && ibdev != NULL)
>  		kiblnd_destroy_dev(ibdev);
>  
>  net_failed:

I think the warning must be for a really really old version.  This was
fixed in: 3247c4e5ef5d ('staging: lustre: lnet: klnds: o2iblnd: fix null
dereference on failed path in o2iblnd.c').

The new NULL check is superflous.

> diff --git a/drivers/staging/lustre/lnet/lnet/lib-move.c b/drivers/staging/lustre/lnet/lnet/lib-move.c
> index 5631f60..7a68382 100644
> --- a/drivers/staging/lustre/lnet/lnet/lib-move.c
> +++ b/drivers/staging/lustre/lnet/lnet/lib-move.c
> @@ -162,6 +162,7 @@ lnet_iov_nob(unsigned int niov, struct kvec *iov)
>  {
>  	unsigned int nob = 0;
>  
> +	LASSERT(niov == 0 || iov);
>  	while (niov-- > 0)
>  		nob += (iov++)->iov_len;
>  
> @@ -280,6 +281,7 @@ lnet_kiov_nob(unsigned int niov, lnet_kiov_t *kiov)
>  {
>  	unsigned int nob = 0;
>  
> +	LASSERT(niov == 0 || kiov);
>  	while (niov-- > 0)
>  		nob += (kiov++)->kiov_len;
>  

Fine, I suppose.

> diff --git a/drivers/staging/lustre/lnet/selftest/conctl.c b/drivers/staging/lustre/lnet/selftest/conctl.c
> index 556c837..2ca7d0e 100644
> --- a/drivers/staging/lustre/lnet/selftest/conctl.c
> +++ b/drivers/staging/lustre/lnet/selftest/conctl.c
> @@ -679,45 +679,46 @@ static int
>  lst_stat_query_ioctl(lstio_stat_args_t *args)
>  {
>  	int rc;
> -	char *name;
> +	char *name = NULL;
>  
>  	/* TODO: not finished */
>  	if (args->lstio_sta_key != console_session.ses_key)
>  		return -EACCES;
>  
> -	if (args->lstio_sta_resultp == NULL ||
> -	    (args->lstio_sta_namep  == NULL &&
> -	     args->lstio_sta_idsp   == NULL) ||
> -	    args->lstio_sta_nmlen <= 0 ||
> -	    args->lstio_sta_nmlen > LST_NAME_SIZE)
> -		return -EINVAL;
> -
> -	if (args->lstio_sta_idsp != NULL &&
> -	    args->lstio_sta_count <= 0)
> +	if (!args->lstio_sta_resultp)
>  		return -EINVAL;
>  
> -	LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
> -	if (name == NULL)
> -		return -ENOMEM;
> -
> -	if (copy_from_user(name, args->lstio_sta_namep,
> -			       args->lstio_sta_nmlen)) {
> -		LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
> -		return -EFAULT;
> -	}
> +	if (args->lstio_sta_idsp) {
> +		if (args->lstio_sta_count <= 0)
> +			return -EINVAL;
>  
> -	if (args->lstio_sta_idsp == NULL) {
> -		rc = lstcon_group_stat(name, args->lstio_sta_timeout,
> -				       args->lstio_sta_resultp);
> -	} else {
>  		rc = lstcon_nodes_stat(args->lstio_sta_count,
>  				       args->lstio_sta_idsp,
>  				       args->lstio_sta_timeout,
>  				       args->lstio_sta_resultp);
> -	}
> +	} else if (args->lstio_sta_namep) {
> +		if (args->lstio_sta_nmlen <= 0 ||
> +		    args->lstio_sta_nmlen > LST_NAME_SIZE)
> +			return -EINVAL;
> +
> +		LIBCFS_ALLOC(name, args->lstio_sta_nmlen + 1);
> +		if (!name)
> +			return -ENOMEM;
>  
> -	LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);
> +		rc = copy_from_user(name, args->lstio_sta_namep,
> +				    args->lstio_sta_nmlen);
> +		if (!rc)
> +			rc = lstcon_group_stat(name, args->lstio_sta_timeout,
> +					       args->lstio_sta_resultp);
> +		else
> +			rc = -EFAULT;
>  
> +	} else {
> +		rc = -EINVAL;
> +	}
> +
> +	if (name)
> +		LIBCFS_FREE(name, args->lstio_sta_nmlen + 1);


There is no bug fix here.  This code was fine when it was merged into
the kernel in 2013 so I have no idea how out of date the static checker
warning is...  The new code doesn't do unnecessary allocations so that's
good but "name" should be declared in the block where it is used instead
of at the start of the function.  Btw, we assume that the user gives us
a NUL terminated string for "name" so we should fix that bug as well.

TODO: lustre: don't assume "name" is NUL terminated

regards,
dan carpenter