Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?utf-8?Q?Lu=C3=ADs_Henriques?= <lhenriques@suse.de>
To:     Eric Biggers <ebiggers@kernel.org>
Cc:     Theodore Ts'o <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] ext4: fix memory leak in ext4_fname_setup_filename()
 error path
In-Reply-To: <20230802042305.GB1543@sol.localdomain> (Eric Biggers's message
        of "Tue, 1 Aug 2023 21:23:05 -0700")
References: <20230801144136.23565-1-lhenriques@suse.de>
        <20230802042305.GB1543@sol.localdomain>
Date:   Wed, 02 Aug 2023 09:12:57 +0100
Message-ID: <87wmydrhfa.fsf@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Eric Biggers <ebiggers@kernel.org> writes:

> On Tue, Aug 01, 2023 at 03:41:36PM +0100, Lu=C3=ADs Henriques wrote:
>> If casefolding the filename fails, we'll be leaking fscrypt_buf name.
>> Make sure we free it in the error path.
>>=20
>> Signed-off-by: Lu=C3=ADs Henriques <lhenriques@suse.de>
>> ---
>>  fs/ext4/crypto.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>=20
>> diff --git a/fs/ext4/crypto.c b/fs/ext4/crypto.c
>> index e20ac0654b3f..9e4503b051c4 100644
>> --- a/fs/ext4/crypto.c
>> +++ b/fs/ext4/crypto.c
>> @@ -33,6 +33,8 @@ int ext4_fname_setup_filename(struct inode *dir, const=
 struct qstr *iname,
>>=20=20
>>  #if IS_ENABLED(CONFIG_UNICODE)
>>  	err =3D ext4_fname_setup_ci_filename(dir, iname, fname);
>> +	if (err)
>> +		fscrypt_free_filename(&name);
>>  #endif
>>  	return err;
>>  }
>
> Doesn't ext4_fname_prepare_lookup() have the same bug?

Doh! I looked into that function too and then completely forgot about it.
It may also call fscrypt_setup_filename(), so it has to do the free in the
error path as well.  I'll send out v2 to include that fix too.

> Also, please include a Fixes tag.

Ok, makes sense.  Thanks!

Cheers,
--=20
Lu=C3=ADs