usercopy: Do not fail on memory from former init sections

PATCH Superseded Helge Deller 2 participants 5 messages Jan 7, 2022 – Jan 8, 2022

Subsystems (2)

Memory management Kernel hardening (not covered by other areas)

lore

Share & tools

lore

Helge Deller

author

2022-01-07 00:20 UTC #0 lore raw

PATCH 1/1 usercopy: Do not fail on memory from former init sections

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 <[email protected]>

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

mm/usercopy.c +15 -0

index b3de3c4eefba..37a35c6051bc 100644
@@ -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);

Andrew Morton

maintainer

2022-01-07 23:44 UTC #1 lore raw

Quoted 10 lines · On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <[email protected]> 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.

Quoted 1 line

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

So is this a must-have for 5.16?

Andrew Morton

maintainer

2022-01-07 23:46 UTC #2 lore raw

In reply to Helge Deller

Quoted 8 lines · On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <[email protected]> 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.

Quoted 6 lines

Fix this failure by first checking against the init sections before checking against the _stext/_etext section.
Signed-off-by Helge Deller <[email protected]>
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?

Andrew Morton

maintainer

2022-01-07 23:51 UTC #3 lore raw

In reply to Helge Deller

Quoted 8 lines · On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <[email protected]> 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.

Quoted 5 lines

Fix this failure by first checking against the init sections before checking against the _stext/_etext section.
Signed-off-by Helge Deller <[email protected]>
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!

Helge Deller

author

2022-01-08 00:24 UTC #4 lore raw

Hi Andrew,

Quoted 10 lines · Andrew Morton

Quoted 8 lines · On Fri, 7 Jan 2022 01:19:24 +0100 Helge Deller <[email protected]> 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.

Quoted 5 lines

Quoted 5 lines
Fix this failure by first checking against the init sections before checking against the _stext/_etext section.
Signed-off-by Helge Deller <[email protected]>
Fixes 98400ad75e95 ("parisc: Fix backtrace to always include init funtion names")

You asked:

Quoted 1 line

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.

Quoted 2 lines

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.

Quoted 1 line

So is this a must-have for 5.16?

No. Current trees are fine.

Quoted 2 lines

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