[PATCH V2] fs: perform the check when page without mapping but page->mapping contains junk or random bitscribble

[PATCH V2] fs: perform the check when page without mapping but page->mapping contains junk or random bitscribble

[xiaosong.ma]

perform the check in dump_mapping() to print warning info and avoid crash with invalid non-NULL page->mapping.
For example, a panic with following backtraces show dump_page will show wrong info and panic when the bad page
is non-NULL mapping and page->mapping is 0x80000000000.

    crash_arm64> bt
    PID: 232    TASK: ffffff80e8c2c340  CPU: 0   COMMAND: "Binder:232_2"
     #0 [ffffffc013e5b080] sysdump_panic_event$b2bce43a479f4f7762201bfee02d7889 at ffffffc0108d7c2c
     #1 [ffffffc013e5b0c0] atomic_notifier_call_chain at ffffffc010300228
     #2 [ffffffc013e5b2c0] panic at ffffffc0102c926c
     #3 [ffffffc013e5b370] die at ffffffc010267670
     #4 [ffffffc013e5b3a0] die_kernel_fault at ffffffc0102808a4
     #5 [ffffffc013e5b3d0] __do_kernel_fault at ffffffc010280820
     #6 [ffffffc013e5b410] do_bad_area at ffffffc01028059c
     #7 [ffffffc013e5b440] do_translation_fault$4df5decbea5d08a63349aa36f07426b2 at ffffffc0111149c8
     #8 [ffffffc013e5b470] do_mem_abort at ffffffc0100a4488
     #9 [ffffffc013e5b5e0] el1_ia at ffffffc0100a6c00
     #10 [ffffffc013e5b5f0] __dump_page at ffffffc0104beecc
     #11 [ffffffc013e5b630] bad_page at ffffffc0104e6ffc
     #12 [ffffffc013e5b820] rmqueue_bulk at ffffffc0104e9128
     #13 [ffffffc013e5b950] rmqueue at ffffffc0104e7c3c
     #14 [ffffffc013e5b9c0] get_page_from_freelist at ffffffc0104e3e3c
     #15 [ffffffc013e5ba50] __alloc_pages_nodemask at ffffffc0104e3a7c
     #16 [ffffffc013e5bac0] pagecache_get_page at ffffffc01047d0e4
     #17 [ffffffc013e5bb20] grab_cache_page_write_begin at ffffffc010480e3c
     #18 [ffffffc013e5bb50] block_write_begin at ffffffc010586204
     #19 [ffffffc013e5bb90] blkdev_write_begin$75b353f60767e771433fc3b19ba260ab at ffffffc01058cc48
     #20 [ffffffc013e5bc00] generic_perform_write at ffffffc010480f1c
     #21 [ffffffc013e5bc60] __generic_file_write_iter at ffffffc01048115c
     #22 [ffffffc013e5bcf0] blkdev_write_iter at ffffffc01058c0a8
     #23 [ffffffc013e5bda0] __vfs_write at ffffffc01052d808
     #24 [ffffffc013e5bdd0] vfs_write at ffffffc01052da5c
     #25 [ffffffc013e5be30] __arm64_sys_pwrite64 at ffffffc01052e09c
     #26 [ffffffc013e5be60] el0_svc_common at ffffffc010272224
     #27 [ffffffc013e5bea0] el0_svc_handler at ffffffc010272148
     #28 [ffffffc013e5bff0] el0_svc at ffffffc0100a7ec4

Signed-off-by: xiaosong.ma <Xiaosong.Ma@unisoc.com>
---
 fs/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index f453eb5..c9021e5 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -564,7 +564,8 @@ void dump_mapping(const struct address_space *mapping)
 	 * If mapping is an invalid pointer, we don't want to crash
 	 * accessing it, so probe everything depending on it carefully.
 	 */
-	if (get_kernel_nofault(host, &mapping->host) ||
+	if (get_kernel_nofault(mapping, &mapping) ||
+	    get_kernel_nofault(host, &mapping->host) ||
 	    get_kernel_nofault(a_ops, &mapping->a_ops)) {
 		pr_warn("invalid mapping:%px\n", mapping);
 		return;
-- 
1.9.1

Re: [PATCH V2] fs: perform the check when page without mapping but page->mapping contains junk or random bitscribble

[Matthew Wilcox]

On Mon, Apr 10, 2023 at 09:45:02AM +0800, xiaosong.ma wrote:
> perform the check in dump_mapping() to print warning info and avoid crash with invalid non-NULL page->mapping.
> For example, a panic with following backtraces show dump_page will show wrong info and panic when the bad page
> is non-NULL mapping and page->mapping is 0x80000000000.
> 
>     crash_arm64> bt
>     PID: 232    TASK: ffffff80e8c2c340  CPU: 0   COMMAND: "Binder:232_2"
>      #0 [ffffffc013e5b080] sysdump_panic_event$b2bce43a479f4f7762201bfee02d7889 at ffffffc0108d7c2c
>      #1 [ffffffc013e5b0c0] atomic_notifier_call_chain at ffffffc010300228
>      #2 [ffffffc013e5b2c0] panic at ffffffc0102c926c
>      #3 [ffffffc013e5b370] die at ffffffc010267670
>      #4 [ffffffc013e5b3a0] die_kernel_fault at ffffffc0102808a4
>      #5 [ffffffc013e5b3d0] __do_kernel_fault at ffffffc010280820
>      #6 [ffffffc013e5b410] do_bad_area at ffffffc01028059c
>      #7 [ffffffc013e5b440] do_translation_fault$4df5decbea5d08a63349aa36f07426b2 at ffffffc0111149c8
>      #8 [ffffffc013e5b470] do_mem_abort at ffffffc0100a4488
>      #9 [ffffffc013e5b5e0] el1_ia at ffffffc0100a6c00
>      #10 [ffffffc013e5b5f0] __dump_page at ffffffc0104beecc

This doesn't show a crash in dump_mapping(), it shows a crash in
__dump_page().

> diff --git a/fs/inode.c b/fs/inode.c
> index f453eb5..c9021e5 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -564,7 +564,8 @@ void dump_mapping(const struct address_space *mapping)
>  	 * If mapping is an invalid pointer, we don't want to crash
>  	 * accessing it, so probe everything depending on it carefully.
>  	 */
> -	if (get_kernel_nofault(host, &mapping->host) ||
> +	if (get_kernel_nofault(mapping, &mapping) ||
> +	    get_kernel_nofault(host, &mapping->host) ||

This patch makes no sense.  Essentially, you're saying
	mapping = &mapping
which is obviously wrong.

Re: [PATCH V2] fs: perform the check when page without mapping but page->mapping contains junk or random bitscribble

[Andrew Morton]

On Tue, 11 Apr 2023 13:16:18 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Apr 10, 2023 at 09:45:02AM +0800, xiaosong.ma wrote:
> > perform the check in dump_mapping() to print warning info and avoid crash with invalid non-NULL page->mapping.
> > For example, a panic with following backtraces show dump_page will show wrong info and panic when the bad page
> > is non-NULL mapping and page->mapping is 0x80000000000.
> > 
> >     crash_arm64> bt
> >     PID: 232    TASK: ffffff80e8c2c340  CPU: 0   COMMAND: "Binder:232_2"
> >      #0 [ffffffc013e5b080] sysdump_panic_event$b2bce43a479f4f7762201bfee02d7889 at ffffffc0108d7c2c
> >      #1 [ffffffc013e5b0c0] atomic_notifier_call_chain at ffffffc010300228
> >      #2 [ffffffc013e5b2c0] panic at ffffffc0102c926c
> >      #3 [ffffffc013e5b370] die at ffffffc010267670
> >      #4 [ffffffc013e5b3a0] die_kernel_fault at ffffffc0102808a4
> >      #5 [ffffffc013e5b3d0] __do_kernel_fault at ffffffc010280820
> >      #6 [ffffffc013e5b410] do_bad_area at ffffffc01028059c
> >      #7 [ffffffc013e5b440] do_translation_fault$4df5decbea5d08a63349aa36f07426b2 at ffffffc0111149c8
> >      #8 [ffffffc013e5b470] do_mem_abort at ffffffc0100a4488
> >      #9 [ffffffc013e5b5e0] el1_ia at ffffffc0100a6c00
> >      #10 [ffffffc013e5b5f0] __dump_page at ffffffc0104beecc
> 
> This doesn't show a crash in dump_mapping(), it shows a crash in
> __dump_page().

um, yes.

But if page->mapping is corrupted, where does __dump_page() dereference it?

The initial patch
(https://lkml.kernel.org/r/1680587425-4683-1-git-send-email-Xiaosong.Ma@unisoc.com)
prevented __dump_page() from calling dump_mapping() if page->mapping is
bad, and that presumably fixed things.


> > diff --git a/fs/inode.c b/fs/inode.c
> > index f453eb5..c9021e5 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -564,7 +564,8 @@ void dump_mapping(const struct address_space *mapping)
> >  	 * If mapping is an invalid pointer, we don't want to crash
> >  	 * accessing it, so probe everything depending on it carefully.
> >  	 */
> > -	if (get_kernel_nofault(host, &mapping->host) ||
> > +	if (get_kernel_nofault(mapping, &mapping) ||
> > +	    get_kernel_nofault(host, &mapping->host) ||
> 
> This patch makes no sense.  Essentially, you're saying
> 	mapping = &mapping
> which is obviously wrong.

We're checking for mapping==junk, so this could be 

	get_kernel_nofault(tmp, mapping)

or go direct to copy_from_kernel_nofault().  We used to have a
probe_kernel_address() for this...

So confusion reigns.  I think making dump_mapping() tolerant of a wild
mapping pointer makes sense, but I don't think we actually know why the
reporter's kernel crashed.

Re: [PATCH V2] fs: perform the check when page without mapping but page->mapping contains junk or random bitscribble

[Matthew Wilcox]

On Tue, Apr 11, 2023 at 05:15:36PM -0700, Andrew Morton wrote:
> On Tue, 11 Apr 2023 13:16:18 +0100 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Mon, Apr 10, 2023 at 09:45:02AM +0800, xiaosong.ma wrote:
> > > perform the check in dump_mapping() to print warning info and avoid crash with invalid non-NULL page->mapping.
> > > For example, a panic with following backtraces show dump_page will show wrong info and panic when the bad page
> > > is non-NULL mapping and page->mapping is 0x80000000000.
> > > 
> > >     crash_arm64> bt
> > >     PID: 232    TASK: ffffff80e8c2c340  CPU: 0   COMMAND: "Binder:232_2"
> > >      #0 [ffffffc013e5b080] sysdump_panic_event$b2bce43a479f4f7762201bfee02d7889 at ffffffc0108d7c2c
> > >      #1 [ffffffc013e5b0c0] atomic_notifier_call_chain at ffffffc010300228
> > >      #2 [ffffffc013e5b2c0] panic at ffffffc0102c926c
> > >      #3 [ffffffc013e5b370] die at ffffffc010267670
> > >      #4 [ffffffc013e5b3a0] die_kernel_fault at ffffffc0102808a4
> > >      #5 [ffffffc013e5b3d0] __do_kernel_fault at ffffffc010280820
> > >      #6 [ffffffc013e5b410] do_bad_area at ffffffc01028059c
> > >      #7 [ffffffc013e5b440] do_translation_fault$4df5decbea5d08a63349aa36f07426b2 at ffffffc0111149c8
> > >      #8 [ffffffc013e5b470] do_mem_abort at ffffffc0100a4488
> > >      #9 [ffffffc013e5b5e0] el1_ia at ffffffc0100a6c00
> > >      #10 [ffffffc013e5b5f0] __dump_page at ffffffc0104beecc
> > 
> > This doesn't show a crash in dump_mapping(), it shows a crash in
> > __dump_page().
> 
> um, yes.
> 
> But if page->mapping is corrupted, where does __dump_page() dereference it?

I don't see anywhere that it does, so I'm suspicious that we have the
correct diagnosis here.

> The initial patch
> (https://lkml.kernel.org/r/1680587425-4683-1-git-send-email-Xiaosong.Ma@unisoc.com)
> prevented __dump_page() from calling dump_mapping() if page->mapping is
> bad, and that presumably fixed things.

Right, but doesn't the _existing_ get_kernel_nofault(host, &mapping->host)
already prevent us from blindly dereferencing a bad mapping pointer?

> > > -	if (get_kernel_nofault(host, &mapping->host) ||
> > > +	if (get_kernel_nofault(mapping, &mapping) ||
> > > +	    get_kernel_nofault(host, &mapping->host) ||
> > 
> > This patch makes no sense.  Essentially, you're saying
> > 	mapping = &mapping
> > which is obviously wrong.
> 
> We're checking for mapping==junk, so this could be 
> 
> 	get_kernel_nofault(tmp, mapping)

Why will that be better than get_kernel_nofault(host, &mapping->host)?
I see no tangible difference between get_kernel_nofault(0x8000'0000) and
get_kernel_nofault(0x8000'0084) (or whatever the offset is).

> or go direct to copy_from_kernel_nofault().  We used to have a
> probe_kernel_address() for this...
> 
> So confusion reigns.  I think making dump_mapping() tolerant of a wild
> mapping pointer makes sense, but I don't think we actually know why the
> reporter's kernel crashed.

In my mind dump_mapping() is already tolerant of a wild page->mapping
pointer.  I think the problem is something entirely different.

Re: [PATCH V2] perform the check when page without mapping but page->mapping contains junk or random bitscribble

[William Kucharski]

Just looking at this and the backtrace:

> On Apr 12, 2023, at 09:14, Matthew Wilcox <willy@infradead.org> wrote:
> 
> On Tue, Apr 11, 2023 at 05:15:36PM -0700, Andrew Morton wrote:
>> On Tue, 11 Apr 2023 13:16:18 +0100 Matthew Wilcox <willy@infradead.org> wrote:
>> 
>>> On Mon, Apr 10, 2023 at 09:45:02AM +0800, xiaosong.ma wrote:
>>>> perform the check in dump_mapping() to print warning info and avoid crash with invalid non-NULL page->mapping.
>>>> For example, a panic with following backtraces show dump_page will show wrong info and panic when the bad page
>>>> is non-NULL mapping and page->mapping is 0x80000000000.
>>>> 
>>>>    crash_arm64> bt
>>>>    PID: 232    TASK: ffffff80e8c2c340  CPU: 0   COMMAND: "Binder:232_2"
>>>>     #0 [ffffffc013e5b080] sysdump_panic_event$b2bce43a479f4f7762201bfee02d7889 at ffffffc0108d7c2c
>>>>     #1 [ffffffc013e5b0c0] atomic_notifier_call_chain at ffffffc010300228
>>>>     #2 [ffffffc013e5b2c0] panic at ffffffc0102c926c
>>>>     #3 [ffffffc013e5b370] die at ffffffc010267670
>>>>     #4 [ffffffc013e5b3a0] die_kernel_fault at ffffffc0102808a4
>>>>     #5 [ffffffc013e5b3d0] __do_kernel_fault at ffffffc010280820
>>>>     #6 [ffffffc013e5b410] do_bad_area at ffffffc01028059c
>>>>     #7 [ffffffc013e5b440] do_translation_fault$4df5decbea5d08a63349aa36f07426b2 at ffffffc0111149c8
>>>>     #8 [ffffffc013e5b470] do_mem_abort at ffffffc0100a4488
>>>>     #9 [ffffffc013e5b5e0] el1_ia at ffffffc0100a6c00
>>>>     #10 [ffffffc013e5b5f0] __dump_page at ffffffc0104beecc
>>> 
>>> This doesn't show a crash in dump_mapping(), it shows a crash in
>>> __dump_page().
>> 
>> um, yes.
>> 
>> But if page->mapping is corrupted, where does __dump_page() dereference it?
> 
> I don't see anywhere that it does, so I'm suspicious that we have the
> correct diagnosis here.

I agree; since dump_mapping() is an actual function rather than a macro or
inline, if a bad dereference were happening within dump_mapping() I would think
we SHOULD see the call to dump_mapping() on the stack unless I'm missing
something obvious here.

Instead I'd like to know which instruction the faulting address in __dump_page()
maps to for the kernel experiencing this.

>> The initial patch
>> (https://lkml.kernel.org/r/1680587425-4683-1-git-send-email-Xiaosong.Ma@unisoc.com)
>> prevented __dump_page() from calling dump_mapping() if page->mapping is
>> bad, and that presumably fixed things.
> 
> Right, but doesn't the _existing_ get_kernel_nofault(host, &mapping->host)
> already prevent us from blindly dereferencing a bad mapping pointer?

I would think it would, but given the traceback, is the fault occurring
within dump_mapping(), or have we perhaps completed dump_mapping() and some
subtle corruption occurred such that the fault occurs on the return to
__dump_page()?

Certainly dump_mapping() looks to do the right thing to avoid using a bad
passed "mapping" as it's not dereferenced anywhere without checks, just
used for pointer math to create an address for calls to get_kernel_notfault().

>> So confusion reigns.  I think making dump_mapping() tolerant of a wild
>> mapping pointer makes sense, but I don't think we actually know why the
>> reporter's kernel crashed.
> 
> In my mind dump_mapping() is already tolerant of a wild page->mapping
> pointer.  I think the problem is something entirely different.

Again, I agree.

As posited above, could it be that something occurs within dump_mapping()
such that when the code returns to __dump_page() it is at THAT point that
the fault occurs? That would explain the backtrace and why it shows the
fault as occurring within __dump_page(), but upon first glance the
mechanism by which this could be occurring eludes me.

The original patch doesn't mention whether any pr_warn() messages were
printed as a result of the call to dump_mapping(), and the suggested fix
would fix the issue whether the fault were occurring within dump_mapping() or
in the return from calling dump_mapping().

    -- Bill