linux-kernel - Re: [PATCH 1/3] ext4: Fix an error handling path in ext4_mb_init

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9383bdd6-ac04-4a14-aec1-bb65b67ace75@stanley.mountain>
Date: Mon, 6 Jan 2025 14:35:37 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc: Theodore Ts'o <tytso@....edu>,
	Andreas Dilger <adilger.kernel@...ger.ca>,
	Alex Tomas <alex@...sterfs.com>, Eric Sandeen <sandeen@...hat.com>,
	"Aneesh Kumar K.V" <aneesh.kumar@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	Andreas Dilger <adilger@...sterfs.com>, linux-ext4@...r.kernel.org
Subject: Re: [PATCH 1/3] ext4: Fix an error handling path in
 ext4_mb_init_cache()

On Fri, Jan 03, 2025 at 02:59:16PM +0100, Christophe JAILLET wrote:
> 'bhs' is an un-initialized pointer.
> If 'groups_per_page' == 1, 'bh' is assigned its address.
> 
> Then, in the for loop below, if we early exit, either because
> "group >= ngroups" or if ext4_get_group_info() fails, then it is still left
> un-initialized.
> 
> It can then be used.
> NULL tests could fail and lead to unexpected behavior. Also, should the
> error handling path be called, brelse() would be passed a potentially
> invalid value.
> 
> Better safe than sorry, just make sure it is correctly initialized to NULL.
> 
> Fixes: c9de560ded61 ("ext4: Add multi block allocator for ext4")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
> ---
> Compile tested only.
> 
> The scenario looks possible, but I don't know if it can really happen...

A pointer to the stack can't ever equal the address of the heap so this
can't happen and it should not have a Fixes tag.

Setting the pointer to NULL probably silences a static checker warning
and these days everyone automatically zeroes stack data so it doesn't
affect the compiled code.  However generally we generally say that we
should fix the checker instead.

I've thought about this in Smatch for a while, and I think what I would
do is say that kmalloc() returns memory that is unique.  Smatch tracks if
variables are equal to each other and unique variables wouldn't be equal
to anything that came earlier.  But I haven't actually tried to implement
this.

regards,
dan carpenter