linux-kernel - Re: [PATCH 5/8] fs/ntfs3: Use kvmalloc instead of kmalloc(... __GFP

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 for Android: free password hash cracker in your pocket

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

Message-ID: <bca8d526-2397-4ca5-b1d6-5758c9334a81@I-love.SAKURA.ne.jp>
Date: Sat, 23 Dec 2023 22:33:11 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Matthew Wilcox <willy@...radead.org>,
        Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
Cc: ntfs3@...ts.linux.dev,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 5/8] fs/ntfs3: Use kvmalloc instead of kmalloc(...
 __GFP_NOWARN)

On 2023/12/23 13:46, Matthew Wilcox wrote:
> On Mon, Jul 03, 2023 at 11:26:36AM +0400, Konstantin Komarov wrote:
>>
>> Signed-off-by: Konstantin Komarov <almaz.alexandrovich@...agon-software.com>
> 
> So, um, why?  I mean, I know what kvmalloc does, but why do you want to
> do it?  Also, this is missing changes from kfree() to kvfree() so if
> you do end up falling back to vmalloc, you'll hit a bug in kfree().

Right. The first thing we should do is to revert commit fc471e39e38f("fs/ntfs3:
Use kvmalloc instead of kmalloc(... __GFP_NOWARN)"), for that commit is horrible.

> 
>> +++ b/fs/ntfs3/attrlist.c
>> @@ -52,7 +52,8 @@ int ntfs_load_attr_list(struct ntfs_inode *ni, struct
>> ATTRIB *attr)
>>
>>      if (!attr->non_res) {
>>          lsize = le32_to_cpu(attr->res.data_size);
>> -        le = kmalloc(al_aligned(lsize), GFP_NOFS | __GFP_NOWARN);
>> +        /* attr is resident: lsize < record_size (1K or 4K) */
>> +        le = kvmalloc(al_aligned(lsize), GFP_KERNEL);

syzbot is reporting that

  /* attr is resident: lsize < record_size (1K or 4K) */

was false at https://syzkaller.appspot.com/bug?extid=f987ceaddc6bcc334cde .
Maybe you can fix this report by adding more validations. Please be
sure to take into account that the filesystem image is corrupted/crafted.

But you can't replace GFP_NOFS with GFP_KERNEL anyway, for syzbot is also
reporting GFP_KERNEL allocation with filesystem lock held
at https://syzkaller.appspot.com/bug?extid=18f543fc90dd1194c616 .

By the way, you might want to add __GFP_RETRY_MAYFAIL when you can't use
GFP_KERNEL, for GFP_NOFS memory allocation request cannot trigger OOM
killer in order to allocate requested amount of memory, leading to
possibility of out-of-memory deadlock. Especially when there is possibility
that kvmalloc() needs to allocate so many pages...
See https://elixir.bootlin.com/linux/v6.7-rc6/source/mm/page_alloc.c#L3458 .