linux-kernel - Re: [PATCH] fs: fix possible extra iput() in do

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]

Date:   Thu, 28 Sep 2023 16:06:40 +0100
From:   Luís Henriques <lhenriques@...e.de>
To:     Mateusz Guzik <mjguzik@...il.com>
Cc:     Alexander Viro <viro@...iv.linux.org.uk>,
        Christian Brauner <brauner@...nel.org>,
        David Howells <dhowells@...hat.com>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fs: fix possible extra iput() in do_unlinkat()

Mateusz Guzik <mjguzik@...il.com> writes:

> On Thu, Sep 28, 2023 at 02:11:29PM +0100, Luís Henriques wrote:
>> Because inode is being initialised before checking if dentry is negative,
>> and the ihold() is only done if the dentry is *not* negative, the cleanup
>> code may end-up doing an extra iput() on that inode.
>> 
>> Fixes: b18825a7c8e3 ("VFS: Put a small type field into struct dentry::d_flags")
>> Signed-off-by: Luís Henriques <lhenriques@...e.de>
>> ---
>> Hi!
>> 
>> I was going to also remove the 'if (inode)' before the 'iput(inode)',
>> because 'iput()' already checks for NULL anyway.  But since I probably
>> wouldn't have caught this bug if it wasn't for that 'if', I decided to
>> keep it there.  But I can send v2 with that change too if you prefer.
>> 
>> Cheers,
>> --
>> Luís
>> 
>>  fs/namei.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 567ee547492b..156a570d7831 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -4386,11 +4386,9 @@ int do_unlinkat(int dfd, struct filename *name)
>>  	if (!IS_ERR(dentry)) {
>>  
>>  		/* Why not before? Because we want correct error value */
>> -		if (last.name[last.len])
>> +		if (last.name[last.len] || d_is_negative(dentry))
>>  			goto slashes;
>>  		inode = dentry->d_inode;
>> -		if (d_is_negative(dentry))
>> -			goto slashes;
>>  		ihold(inode);
>>  		error = security_path_unlink(&path, dentry);
>>  		if (error)
>
> I ran into this myself, but I'm pretty sure there is no bug here. The
> code is just incredibly misleading and it became this way from the
> sweeping change introducing d_is_negative. I could not be bothered to
> argue about patching it so I did not do anything. ;)
>
> AFAICS it is an invariant that d_is_negative passes iff d_inode is NULL.

Ah! yes, of course.  Makes sense.  Thanks for your review.

> Personally I support the patch, but commit message needs to stop
> claiming a bug.

OK, I'll rephrase the commit message for v2 so that it's clear it's not
really fixing anything, it just helps clarifying some misleading code
paths.  (Although, to be fair, the commit message doesn't really
explicitly call it a bug :-) ).

Cheers,
-- 
Luís