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: <8bb6722b-52d8-4585-8377-194c241462f1@gmail.com>
Date: Tue, 15 Oct 2024 13:57:50 +0200
From: Gianfranco Trad <gianf.trad@...il.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: akpm@...ux-foundation.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org, skhan@...uxfoundation.org,
 syzbot+4089e577072948ac5531@...kaller.appspotmail.com
Subject: Re: [PATCH v2] mm: fix null pointer dereference in read_cache_folio

On 04/10/24 14:07, Gianfranco Trad wrote:
> On 30/09/24 20:14, Matthew Wilcox wrote:
>> On Mon, Sep 30, 2024 at 11:02:26AM +0200, Gianfranco Trad wrote:
>>> @@ -2360,6 +2360,8 @@ static int filemap_read_folio(struct file 
>>> *file, filler_t filler,
>>>       /* Start the actual read. The read will unlock the page. */
>>>       if (unlikely(workingset))
>>>           psi_memstall_enter(&pflags);
>>> +    if (!filler)
>>> +        return -EIO;
>>
>> This is definitely wrong because you enter memstall, but do not exit it.
> 
> Got it, thanks.
> 
>>
>> As Andrew says, the underlying problem is that the filesystem does not
>> implement ->read_folio.  Which filesystem is this?
> 
> Reproducer via procfs accesses a bpf map backed by an anonymous
> inode (anon_inode_fs_type), with mapping->a_ops pointing to anon_aops,
> hence, read_folio() undefined.
> 
>>
>>>       error = filler(file, folio);
>>>       if (unlikely(workingset))
>>>           psi_memstall_leave(&pflags);
>>> -- 
>>> 2.43.0
>>>
> 
> I suppose the next step would be to contact the proper maintainers(?)
> If you have any additional suggestions, I'd be more than glad to listen.
> 
> Thanks to both of you for your time,
> 
> --Gian
> 

Hello,

While studying how to implement read_folio in anon_aops for this 
specific case (bpf map backed by anon_inode_fs_type) I've come up with 
an intermediate solution that mitigates the null pointer dereference and 
avoids the memstall issue (compared to my previous patch) immediately, 
for all filesystems that do not implement read_folio in their 
address_space_operations.

The patch [1] looks like this:

diff --git a/mm/filemap.c b/mm/filemap.c
index 4f3753f0a158..680d98086c00 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -3775,6 +3775,8 @@ static struct folio *do_read_cache_folio(struct 
address_space *mapping,
  	struct folio *folio;
  	int err;

+	if (!filler && (!mapping->a_ops || !mapping->a_ops->read_folio))
+		return ERR_PTR(-ENOSYS);
  	if (!filler)
  		filler = mapping->a_ops->read_folio;
  repeat:

Patch was already tested with syzbot on the same reproducer case. 
Reproducer did not trigger any issue [2].

Let me know if for now this patch looks good enough, therefore I'll send 
it to you, or if I should work on it more.

Thanks for your time,

[1] https://syzkaller.appspot.com/text?tag=Patch&x=142e045f980000
[2] https://syzkaller.appspot.com/x/log.txt?x=1551045f980000

-- Gian



Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ