| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20151202074617.GI18797@mwanda>
Date: Wed, 2 Dec 2015 10:46:17 +0300
From: Dan Carpenter <dan.carpenter@...cle.com>
To: James Simmons <jsimmons@...radead.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
devel@...verdev.osuosl.org, Oleg Drokin <oleg.drokin@...el.com>,
Andreas Dilger <andreas.dilger@...el.com>,
Sebastien Buisson <sebastien.buisson@...l.net>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
lustre-devel@...ts.lustre.org
Subject: Re: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference'
errors for LNet
On Fri, Nov 20, 2015 at 06:35:38PM -0500, James Simmons wrote:
> From: Sebastien Buisson <sebastien.buisson@...l.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@...l.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@...el.com>
> Reviewed-by: Oleg Drokin <oleg.drokin@...el.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists