* [PATCH v1] Fix NULL pointer dereference in read_cache_folio @ 2024-09-29 23:05 Gianfranco Trad 2024-09-30 9:02 ` [PATCH v2] " Gianfranco Trad 0 siblings, 1 reply; 6+ messages in thread From: Gianfranco Trad @ 2024-09-29 23:05 UTC (permalink / raw) To: willy, akpm Cc: linux-fsdevel, linux-mm, linux-kernel, skhan, Gianfranco Trad, syzbot+4089e577072948ac5531 Add check on filler to prevent NULL pointer dereference condition in read_cache_folio[1]. [1] https://syzkaller.appspot.com/bug?extid=4089e577072948ac5531 Reported-by: syzbot+4089e577072948ac5531@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=4089e577072948ac5531 Tested-by: syzbot+4089e577072948ac5531@syzkaller.appspotmail.com Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> --- mm/filemap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/mm/filemap.c b/mm/filemap.c index 4f3753f0a158..960f389e2d3b 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2360,7 +2360,10 @@ 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); - error = filler(file, folio); + if (filler) + error = filler(file, folio); + else + return -EIO; if (unlikely(workingset)) psi_memstall_leave(&pflags); if (error) -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2] Fix NULL pointer dereference in read_cache_folio 2024-09-29 23:05 [PATCH v1] Fix NULL pointer dereference in read_cache_folio Gianfranco Trad @ 2024-09-30 9:02 ` Gianfranco Trad 2024-09-30 17:02 ` Andrew Morton 2024-09-30 18:14 ` Matthew Wilcox 0 siblings, 2 replies; 6+ messages in thread From: Gianfranco Trad @ 2024-09-30 9:02 UTC (permalink / raw) To: gianf.trad Cc: akpm, linux-fsdevel, linux-kernel, linux-mm, skhan, syzbot+4089e577072948ac5531, willy Add check on filler to prevent NULL pointer dereference condition in read_cache_folio[1]. [1] https://syzkaller.appspot.com/bug?extid=4089e577072948ac5531 Reported-by: syzbot+4089e577072948ac5531@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=4089e577072948ac5531 Tested-by: syzbot+4089e577072948ac5531@syzkaller.appspotmail.com Signed-off-by: Gianfranco Trad <gianf.trad@gmail.com> --- Notes: changes in v2: - refactored check on filler. mm/filemap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mm/filemap.c b/mm/filemap.c index 4f3753f0a158..88de8029133c 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -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; error = filler(file, folio); if (unlikely(workingset)) psi_memstall_leave(&pflags); -- 2.43.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Fix NULL pointer dereference in read_cache_folio 2024-09-30 9:02 ` [PATCH v2] " Gianfranco Trad @ 2024-09-30 17:02 ` Andrew Morton 2024-09-30 18:14 ` Matthew Wilcox 1 sibling, 0 replies; 6+ messages in thread From: Andrew Morton @ 2024-09-30 17:02 UTC (permalink / raw) To: Gianfranco Trad Cc: linux-fsdevel, linux-kernel, linux-mm, skhan, syzbot+4089e577072948ac5531, willy On Mon, 30 Sep 2024 11:02:26 +0200 Gianfranco Trad <gianf.trad@gmail.com> wrote: > Add check on filler to prevent NULL pointer dereference condition in > read_cache_folio[1]. > > [1] https://syzkaller.appspot.com/bug?extid=4089e577072948ac5531 Test case https://syzkaller.appspot.com/x/repro.c?x=10a0d880580000 > diff --git a/mm/filemap.c b/mm/filemap.c > index 4f3753f0a158..88de8029133c 100644 > --- a/mm/filemap.c > +++ b/mm/filemap.c > @@ -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; > error = filler(file, folio); > if (unlikely(workingset)) > psi_memstall_leave(&pflags); do_read_cache_folio() already handles a NULL filler (from freader_get_folio()'s read_cache_folio() call). if (!filler) filler = mapping->a_ops->read_folio; so I'm suspecting that an appropriate fix is to teach the underlying address_space_operations (appears to be from /proc/pid/maps) to implement ->read_folio(). ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Fix NULL pointer dereference in read_cache_folio 2024-09-30 9:02 ` [PATCH v2] " Gianfranco Trad 2024-09-30 17:02 ` Andrew Morton @ 2024-09-30 18:14 ` Matthew Wilcox 2024-10-04 12:07 ` Gianfranco Trad 1 sibling, 1 reply; 6+ messages in thread From: Matthew Wilcox @ 2024-09-30 18:14 UTC (permalink / raw) To: Gianfranco Trad Cc: akpm, linux-fsdevel, linux-kernel, linux-mm, skhan, syzbot+4089e577072948ac5531 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. As Andrew says, the underlying problem is that the filesystem does not implement ->read_folio. Which filesystem is this? > error = filler(file, folio); > if (unlikely(workingset)) > psi_memstall_leave(&pflags); > -- > 2.43.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] Fix NULL pointer dereference in read_cache_folio 2024-09-30 18:14 ` Matthew Wilcox @ 2024-10-04 12:07 ` Gianfranco Trad 2024-10-15 11:57 ` [PATCH v2] mm: fix null " Gianfranco Trad 0 siblings, 1 reply; 6+ messages in thread From: Gianfranco Trad @ 2024-10-04 12:07 UTC (permalink / raw) To: Matthew Wilcox Cc: akpm, linux-fsdevel, linux-kernel, linux-mm, skhan, syzbot+4089e577072948ac5531 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] mm: fix null pointer dereference in read_cache_folio 2024-10-04 12:07 ` Gianfranco Trad @ 2024-10-15 11:57 ` Gianfranco Trad 0 siblings, 0 replies; 6+ messages in thread From: Gianfranco Trad @ 2024-10-15 11:57 UTC (permalink / raw) To: Matthew Wilcox Cc: akpm, linux-fsdevel, linux-kernel, linux-mm, skhan, syzbot+4089e577072948ac5531 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-10-15 11:57 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-09-29 23:05 [PATCH v1] Fix NULL pointer dereference in read_cache_folio Gianfranco Trad 2024-09-30 9:02 ` [PATCH v2] " Gianfranco Trad 2024-09-30 17:02 ` Andrew Morton 2024-09-30 18:14 ` Matthew Wilcox 2024-10-04 12:07 ` Gianfranco Trad 2024-10-15 11:57 ` [PATCH v2] mm: fix null " Gianfranco Trad
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).