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

[Simmons, James A.]

>> 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

Ugh. I see breakage everywhere in this code :-( Need to address.  I think we
should convert that to strcpy_to_user as well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Simmons, James A.]

>> 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

Ugh. I see breakage everywhere in this code :-( Need to address.  I think we
should convert that to strcpy_to_user as well.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Dan Carpenter]

On Fri, Nov 20, 2015 at 06:35:38PM -0500, James Simmons wrote:
> From: Sebastien Buisson 
> 
> 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 
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2217
> Reviewed-on: http://review.whamcloud.com/4720
> Reviewed-by: Dmitry Eremin 
> Reviewed-by: Oleg Drokin 
> ---
>  .../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)
> +   

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

[Dan Carpenter]

On Fri, Nov 20, 2015 at 06:35:38PM -0500, James Simmons wrote:
> From: Sebastien Buisson 
> 
> 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 
> Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-2217
> Reviewed-on: http://review.whamcloud.com/4720
> Reviewed-by: Dmitry Eremin 
> Reviewed-by: Oleg Drokin 
> ---
>  .../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,