Message-ID: <YfLpNkmlvoR8iPcq@ls3530>
Date:   Thu, 27 Jan 2022 19:49:26 +0100
From:   Helge Deller <deller@....de>
To:     Andrew Morton <akpm@...ux-foundation.org>,
        linux-csky@...r.kernel.org, linux-mm@...ck.org,
        Linux Kernel <linux-kernel@...r.kernel.org>,
        Guo Ren <guoren@...nel.org>
Cc:     linux-parisc@...r.kernel.org
Subject: [PATCH v2] usercopy/csky: Do not fail on memory from former init
 sections

While working on the parisc port I suddenly noticed that with
HARDENED_USERCOPY=y the usercopy checks randomly reported errors which even
prevented the kernel to boot into userspace.

Specifically the function check_kernel_text_object() reported those errors, as
it thought the kernel variable I was using belongs to the kernel text area, and
as such was invalid to be used.

Looking at the code you'll see that check_kernel_text_object() checks
if the given pointer is inside the _stext to _etext memory area:

/* Is this address range in the kernel text area? */
static inline void check_kernel_text_object(const unsigned long ptr, unsigned long n, bool to_user)
{
        unsigned long textlow = (unsigned long)_stext;
        unsigned long texthigh = (unsigned long)_etext;

        if (overlaps(ptr, n, textlow, texthigh))
                usercopy_abort("kernel text", NULL, to_user, ptr - textlow, n);
}

When I faced the issues on parisc, the parisc kernel memory layout in
vmlinux.lds.S file was the following (it isn't any longer because of commit
98400ad75e95):

        _stext = .;
        __init_begin = .;
        HEAD_TEXT_SECTION
        INIT_DATA_SECTION(PAGE_SIZE)
        __init_end = .;

        _text = .;              /* Text and read-only data */
        .text ALIGN(PAGE_SIZE) : {
                TEXT_TEXT
                LOCK_TEXT
	...
	_etext = .;

As can be seen, the INIT sections are inside the _stext ... _etext section,
and this is important: On parisc the init sections can be quite big when
HUGE_PAGES are enabled and as such triggers the problem more frequently.

Now, after bootup all architectures call free_initmem() which frees up the INIT
section, so the former INIT memory area can be used by the kernel for kmalloc()
or similiar allocations.

Now, when starting to execute userspace the parisc kernel allocated memory to
interact with userspace, and sometimes - by accident - some of that memory was
allocated from the former INIT section.

By using copy_to_user() and copy_from_user() the hardened usercopy checks were
executed and they then triggered the failure in the check_kernel_text_object()
function in case such memory from the INIT section was used. The problem is,
that check_kernel_text_object() currently ignores that the INIT section (which
is now OK to be used) *might* be inside the _stext ...  _etext area.

On parisc I fixed this issue by moving the _stext symbol behind the INIT section.

I checked the vmlinux.lds.S files of the other architectures and I do believe that
the sky architecture is still suffering from this problem (maybe unnoticed yet,
because it may happen very seldom/randomly and depending on the size of the INIT
sections).

The current csky vmlinux.lds.S file looks like this:

        _stext = .;
        __init_begin = .;
        INIT_TEXT_SECTION(PAGE_SIZE)
        INIT_DATA_SECTION(PAGE_SIZE)
        __init_end = .;
        .text : AT(ADDR(.text) - LOAD_OFFSET) {
...
        } = 0
        _etext = .;

So, for csky I think the _stext symbol needs to be moved after the __init_end
symbol to prevent the userchecks to trigger.

The other (and in my opinion better) solution is to generally enhance the
usercopy check in check_kernel_text_object() like it's proposed in the patch
below.

Helge

------------------

The memory area between the _stext and the _etext symbols may include the init
sections. Currently this is true for the csky architecture only.
If the init sections are freed after bootup, the kernel may reuse this memory.

In one test the usercopy checks if a given kernel address is inside the .text
section (from _stext to _etext), and it then wrongly fails if the memory is
from the former init sections.

Fix this potential failure by first checking against the init sections before
checking against the _stext/_etext section.

Signed-off-by: Helge Deller <deller@....de>

diff --git a/mm/usercopy.c b/mm/usercopy.c
index b3de3c4eefba..37a35c6051bc 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -113,6 +113,15 @@ static bool overlaps(const unsigned long ptr, unsigned long n,
 	return true;
 }

+static bool inside_init_area(const unsigned long ptr, unsigned long n,
+		char *start, char *end)
+{
+	unsigned long initlow = (unsigned long) start;
+	unsigned long inithigh = (unsigned long) end;
+
+	return (ptr >= initlow && (ptr + n) < inithigh);
+}
+
 /* Is this address range in the kernel text area? */
 static inline void check_kernel_text_object(const unsigned long ptr,
 					    unsigned long n, bool to_user)
@@ -121,6 +130,12 @@ static inline void check_kernel_text_object(const unsigned long ptr,
 	unsigned long texthigh = (unsigned long)_etext;
 	unsigned long textlow_linear, texthigh_linear;

+	/* Ok if inside the former init sections */
+	if (inside_init_area(ptr, n, __init_begin, __init_end))
+		return;
+	if (inside_init_area(ptr, n, _sinittext, _einittext))
+		return;
+
 	if (overlaps(ptr, n, textlow, texthigh))
 		usercopy_abort("kernel text", NULL, to_user, ptr - textlow, n);

Powered by blists - more mailing lists