linux-kernel - RE: [HPDD-discuss] [PATCH] staging: lustre: lustre: include: lustre_update.h: Fix for possible null pointer dereference

Open Source and information security mailing list archives

Date:	Sat, 3 Jan 2015 01:29:20 +0000
From:	Patrick Farrell <paf@...y.com>
To:	Ashley Pittman <apittman@....com>,
	Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
CC:	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"HPDD-discuss@...ts.01.org" <HPDD-discuss@...ts.01.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [HPDD-discuss] [PATCH] staging: lustre: lustre:	include:
	lustre_update.h: Fix for possible null pointer dereference

Ashley,

I sort of understand your larger point, but in this case, I think the purpose of the assert is much better served by the move Rickard is suggesting.  Otherwise only one of its conditions will ever trigger.  It's not that different to die on the assertion or from a null dereference, but I think it's marginally better to fail the assertion.  And it definitely doesn't make sense to have it there and never triggered, which it was before.

- Patrick
________________________________________
From: HPDD-discuss [hpdd-discuss-bounces@...ts.01.org] on behalf of Ashley Pittman [apittman@....com]
Sent: Friday, January 02, 2015 11:55 AM
To: Rickard Strandqvist
Cc: devel@...verdev.osuosl.org; HPDD-discuss@...ts.01.org; Greg Kroah-Hartman; linux-kernel@...r.kernel.org
Subject: Re: [HPDD-discuss] [PATCH] staging: lustre: lustre:    include:        lustre_update.h: Fix for possible null pointer dereference

Rickard,

> On 21 Dec 2014, at 22:43, Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se> wrote:
>
> The NULL check was done to late, and there it was a risk
> of a possible null pointer dereference.
>
> This was partially found by using a static code analysis program called cppcheck.
>
> Signed-off-by: Rickard Strandqvist <rickard_strandqvist@...ctrumdigital.se>
> ---
> drivers/staging/lustre/lustre/include/lustre_update.h |    4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/lustre/lustre/include/lustre_update.h b/drivers/staging/lustre/lustre/include/lustre_update.h
> index 84defce..00e1361 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_update.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_update.h
> @@ -165,12 +165,14 @@ static inline int update_get_reply_buf(struct update_reply *reply, void **buf,
>       int  result;
>
>       ptr = update_get_buf_internal(reply, index, &size);
> +
> +     LASSERT((ptr != NULL && size >= sizeof(int)));
> +
>       result = *(int *)ptr;
>
>       if (result < 0)
>               return result;
>
> -     LASSERT((ptr != NULL && size >= sizeof(int)));

This looks odd to me, LASSERT is essentially BUG_ON() so is used for checking logic bugs.  Moving LASSERT calls doesn’t seem the correct way of resolving a logic problem and if you’re doing static analysis it might be more productive to do it this with LASSERT disabled.

>       *buf = ptr + sizeof(int);
>       return size - sizeof(int);
> }
> --
> 1.7.10.4
>
> _______________________________________________
> HPDD-discuss mailing list
> HPDD-discuss@...ts.01.org
> https://lists.01.org/mailman/listinfo/hpdd-discuss

_______________________________________________
HPDD-discuss mailing list
HPDD-discuss@...ts.01.org
https://lists.01.org/mailman/listinfo/hpdd-discuss
--
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/

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