[PATCH] usercopy: Do not fail on memory from former init sections

[PATCH] usercopy: Do not fail on memory from former init sections

[Helge Deller]

On some platforms the memory area between the _stext and the _etext
symbols includes the init sections (parisc and csky). If the init
sections are freed after bootup, the kernel may reuse this memory.

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

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

Signed-off-by: Helge Deller <deller@gmx.de>
Fixes: 98400ad75e95 ("parisc: Fix backtrace to always include init funtion names")

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);

Re: [PATCH] usercopy: Do not fail on memory from former init sections

[Andrew Morton]

On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <deller@gmx.de> wrote:

> On some platforms the memory area between the _stext and the _etext
> symbols includes the init sections (parisc and csky). If the init
> sections are freed after bootup, the kernel may reuse this memory.
> 
> In one test the usercopy checks if the given address is inside the .text
> section (from _stext to _etext), and it wrongly fails on the mentioned
> platforms if the memory is from the former init section.
> 
> Fix this failure by first checking against the init sections before
> checking against the _stext/_etext section.

This sounds like it might have very serious runtime effects?

Please always fully describe a bug's runtime effects when fixing that bug.

> Fixes: 98400ad75e95 ("parisc: Fix backtrace to always include init funtion names")

So is this a must-have for 5.16?

Re: [PATCH] usercopy: Do not fail on memory from former init sections

[Andrew Morton]

On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <deller@gmx.de> wrote:

> On some platforms the memory area between the _stext and the _etext
> symbols includes the init sections (parisc and csky). If the init
> sections are freed after bootup, the kernel may reuse this memory.
> 
> In one test the usercopy checks if the given address is inside the .text
> section (from _stext to _etext), and it wrongly fails on the mentioned
> platforms if the memory is from the former init section.
> 
> Fix this failure by first checking against the init sections before
> checking against the _stext/_etext section.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Fixes: 98400ad75e95 ("parisc: Fix backtrace to always include init funtion names")
> 

And 98400ad75e95 has cc:stable so we'll want cc:stable on this patch
also, yes?

Re: [PATCH] usercopy: Do not fail on memory from former init sections

[Andrew Morton]

On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <deller@gmx.de> wrote:

> On some platforms the memory area between the _stext and the _etext
> symbols includes the init sections (parisc and csky). If the init
> sections are freed after bootup, the kernel may reuse this memory.
> 
> In one test the usercopy checks if the given address is inside the .text
> section (from _stext to _etext), and it wrongly fails on the mentioned
> platforms if the memory is from the former init section.
> 
> Fix this failure by first checking against the init sections before
> checking against the _stext/_etext section.
> 
> Signed-off-by: Helge Deller <deller@gmx.de>
> Fixes: 98400ad75e95 ("parisc: Fix backtrace to always include init funtion names")

Wait.  98400ad75e95 is actually called

	Revert "parisc: Fix backtrace to always include init funtion names"

and it reverts 279917e27edc2.  This isn't making a lot of sense.


And neither 98400ad75e95 nor 279917e27edc2 touch csky.

And I really wouldn't want to jam a patch into mm/usercopy.c at this
point in the life of 5.16 anyway.

I'll drop this patch.  Please revisit and clarify all these things.  A lot!

Re: [PATCH] usercopy: Do not fail on memory from former init sections

[Helge Deller]

Hi Andrew,

On 1/8/22 00:51, Andrew Morton wrote:
> On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <deller@gmx.de> wrote:
>
>> On some platforms the memory area between the _stext and the _etext
>> symbols includes the init sections (parisc and csky). If the init
>> sections are freed after bootup, the kernel may reuse this memory.
>>
>> In one test the usercopy checks if the given address is inside the .text
>> section (from _stext to _etext), and it wrongly fails on the mentioned
>> platforms if the memory is from the former init section.
>>
>> Fix this failure by first checking against the init sections before
>> checking against the _stext/_etext section.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>> Fixes: 98400ad75e95 ("parisc: Fix backtrace to always include init funtion names")

You asked:
> This sounds like it might have very serious runtime effects?

Yes, the patch fixes very negative effects in the sense that the kernel fails to boot
on parisc if the following patch is included (and if userchecks are enabled):
279917e27edc - parisc: Fix backtrace to always include init funtion names

That patch (279917e27edc) was once in v5.16, but because of the failure
I *reverted* it again with:
98400ad75e95 - Revert "parisc: Fix backtrace to always include init funtion names"

Both patches were originally tagged for stable.
So, all current trees are fine as they are right now as they are.

> And 98400ad75e95 has cc:stable so we'll want cc:stable on this patch
> also, yes?

We could push it to stable if we think it's worth it.
But I'm fine if we simply start fresh with kernel 5.17.
So: no.

> So is this a must-have for 5.16?

No. Current trees are fine.

> And I really wouldn't want to jam a patch into mm/usercopy.c at this
> point in the life of 5.16 anyway.

Right.
I sent the patch for review and for v5.17 (it didn't had a stable tag).

It would be nice if you could queue it up for v5.17.

Helge