linux-kernel - Re: [PATCH] mm: check mapping addr is correct when dump page

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: <20230404141722.45dbdc377e7e1547e302ffa5@linux-foundation.org>
Date:   Tue, 4 Apr 2023 14:17:22 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     "xiaosong.ma" <Xiaosong.Ma@...soc.com>
Cc:     <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Zhaoyang Huang <huangzhaoyang@...il.com>, <ke.wang@...soc.com>
Subject: Re: [PATCH] mm: check mapping addr is correct when dump page

On Tue, 4 Apr 2023 13:50:25 +0800 "xiaosong.ma" <Xiaosong.Ma@...soc.com> wrote:

> when we debug with slub_debug_on, the following backtraces show dump_page
> will show wrong info when the bad page is non-NULL mapping and page->mapping
> is 0x80000000000 so do virt_addr valid check is needed when dump mapping page.

How did this page get ->mapping=0x80000000?  I don't recall anywhere
where we deliberately set this state.

Maybe a random bitscribble?  I guess being defensive in __dump_page()
is sensible - we have reason to believe that the page is in some bad
state.

> --- a/mm/debug.c
> +++ b/mm/debug.c
> @@ -109,7 +109,7 @@ static void __dump_page(struct page *page)
>                 type = "ksm ";
>         else if (PageAnon(page))
>                 type = "anon ";
> -       else if (mapping)
> +       else if (mapping && virt_addr_valid(mapping))
>                 dump_mapping(mapping);

I expect the user will be interested in knowing that ->mapping contains
junk, so perhaps we should print some information telling them this.

In which case, dump_mapping() would be a better place to perform the
check.

And lo, dump_mapping() already did this, so I think all we need is

--- a/fs/inode.c~a
+++ a/fs/inode.c
@@ -565,7 +565,8 @@ void dump_mapping(const struct address_s
 	 * 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) ||
+	    get_kernel_nofault(host, &mapping->host) ||
 	    get_kernel_nofault(a_ops, &mapping->a_ops)) {
 		pr_warn("invalid mapping:%px\n", mapping);
 		return;
_