From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754177AbbLOSIs (ORCPT ); Tue, 15 Dec 2015 13:08:48 -0500 Received: from mta02.ornl.gov ([128.219.177.12]:54772 "EHLO mta02.ornl.gov" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753528AbbLOSIq convert rfc822-to-8bit (ORCPT ); Tue, 15 Dec 2015 13:08:46 -0500 X-SG: RELAYLIST X-IronPort-AV: E=Sophos;i="5.20,433,1444708800"; d="scan'208";a="94303193" From: "Simmons, James A." To: "'Dan Carpenter'" , James Simmons CC: "devel@driverdev.osuosl.org" , "Sebastien Buisson" , Andreas Dilger , Greg Kroah-Hartman , Linux Kernel Mailing List , Oleg Drokin , "lustre-devel@lists.lustre.org" Subject: RE: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet Thread-Topic: [PATCH 02/40] staging: lustre: fix 'NULL pointer dereference' errors for LNet Thread-Index: AQHRLNWkIqJCIOXKb0yadD7aaKBa3Z7MbHoA Date: Tue, 15 Dec 2015 18:08:44 +0000 Message-ID: <2d1f3c4a4b50489896fcc6f703768bb0@EXCHCS32.ornl.gov> References: <1448062576-23757-1-git-send-email-jsimmons@infradead.org> <1448062576-23757-3-git-send-email-jsimmons@infradead.org> <20151202074617.GI18797@mwanda> In-Reply-To: <20151202074617.GI18797@mwanda> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [128.219.12.132] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org >> 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.