linux-kernel - Re: [PATCH] ubifs: Fix memory leak 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] [day] [month] [year] [list]

Message-ID: <585bbddc-7c02-bc10-df37-6b91337e5618@axis.com>
Date:   Thu, 30 Mar 2023 16:38:34 +0200
From:   Mårten Lindahl <martenli@...s.com>
To:     Zhihao Cheng <chengzhihao1@...wei.com>,
        Mårten Lindahl <marten.lindahl@...s.com>,
        Richard Weinberger <richard@....at>
CC:     <linux-mtd@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <kernel@...s.com>
Subject: Re: [PATCH] ubifs: Fix memory leak in do_rename

Hi Zhihao!

On 3/30/23 16:17, Zhihao Cheng wrote:
>> If renaming a file in an encrypted directory, function
>> fscrypt_setup_filename allocates memory for a file name. This name is
>> never used, and before returning to the caller the memory for it is not
>> freed.
>>
>> When running kmemleak on it we see that it is registered as a leak. The
>> report below is triggered by a simple program 'rename' that renames a
>> file in an encrypted directory:
>>
>>    unreferenced object 0xffff888101502840 (size 32):
>>      comm "rename", pid 9404, jiffies 4302582475 (age 435.735s)
>>      backtrace:
>>        __kmem_cache_alloc_node
>>        __kmalloc
>>        fscrypt_setup_filename
>>        do_rename
>>        ubifs_rename
>>        vfs_rename
>>        do_renameat2
>>
>> To fix this we can remove the call to fscrypt_setup_filename as it's not
>> needed.
>>
>> Reported-by: Zhihao Cheng <chengzhihao1@...wei.com>
>> Signed-off-by: Mårten Lindahl <marten.lindahl@...s.com>
>> ---
>>   fs/ubifs/dir.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>
> I'm okay with this modification. It would be better to add a fix tag. 
> (Fixes: 278d9a243635f26 ("ubifs: Rename whiteout atomically"))
>
>
> Reviewed-by: Zhihao Cheng <chengzhihao1@...wei.com>

Thanks! I'll send a v2 with the Fixes tag and your Reviewed-by tag.

Kind regards

Mårten

>
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index 0f29cf201136..4c2dda1346e7 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -358,7 +358,6 @@ static struct inode *create_whiteout(struct inode 
>> *dir, struct dentry *dentry)
>>       umode_t mode = S_IFCHR | WHITEOUT_MODE;
>>       struct inode *inode;
>>       struct ubifs_info *c = dir->i_sb->s_fs_info;
>> -    struct fscrypt_name nm;
>>         /*
>>        * Create an inode('nlink = 1') for whiteout without updating 
>> journal,
>> @@ -369,10 +368,6 @@ static struct inode *create_whiteout(struct 
>> inode *dir, struct dentry *dentry)
>>       dbg_gen("dent '%pd', mode %#hx in dir ino %lu",
>>           dentry, mode, dir->i_ino);
>>   -    err = fscrypt_setup_filename(dir, &dentry->d_name, 0, &nm);
>> -    if (err)
>> -        return ERR_PTR(err);
>> -
>>       inode = ubifs_new_inode(c, dir, mode, false);
>>       if (IS_ERR(inode)) {
>>           err = PTR_ERR(inode);
>> @@ -395,7 +390,6 @@ static struct inode *create_whiteout(struct inode 
>> *dir, struct dentry *dentry)
>>       make_bad_inode(inode);
>>       iput(inode);
>>   out_free:
>> -    fscrypt_free_filename(&nm);
>>       ubifs_err(c, "cannot create whiteout file, error %d", err);
>>       return ERR_PTR(err);
>>   }
>>
>> ---
>> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
>> change-id: 20230330-memleak_rename-fix-6f94a6f99350
>>
>> Best regards,
>>
>