diff mbox series

mm,memblock: reset memblock.reserved to system init state to prevent UAF

Message ID 20230719154137.732d8525@imladris.surriel.com
State New
Headers
Series mm,memblock: reset memblock.reserved to system init state to prevent UAF |

Commit Message

Rik van Riel July 19, 2023, 7:41 p.m. UTC 
  The memblock_discard function frees the memblock.reserved.regions
array, which is good.

However, if a subsequent memblock_free (or memblock_phys_free) comes
in later, from for example ima_free_kexec_buffer, that will result in
a use after free bug in memblock_isolate_range.

When running a kernel with CONFIG_KASAN enabled, this will cause a
kernel panic very early in boot. Without CONFIG_KASAN, there is
a chance that memblock_isolate_range might scribble on memory
that is now in use by somebody else.

Avoid those issues by making sure that memblock_discard points
memblock.reserved.regions back at the static buffer.

If memblock_discard is called while there is still memory
in the memblock.reserved type, that will print a warning
in memblock_remove_region.

Signed-off-by: Rik van Riel <riel@surriel.com>
---
 mm/memblock.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Mike Rapoport July 20, 2023, 5 a.m. UTC | #1 
Hi Ric,

On Wed, Jul 19, 2023 at 03:41:37PM -0400, Rik van Riel wrote:
> The memblock_discard function frees the memblock.reserved.regions
> array, which is good.
> 
> However, if a subsequent memblock_free (or memblock_phys_free) comes
> in later, from for example ima_free_kexec_buffer, that will result in
> a use after free bug in memblock_isolate_range.

The use of memblock_phys_free() in ima_free_kexec_buffer() is entirely
bogus and must be fixed. It should be memblock_free_late() there.
 
> When running a kernel with CONFIG_KASAN enabled, this will cause a
> kernel panic very early in boot. Without CONFIG_KASAN, there is
> a chance that memblock_isolate_range might scribble on memory
> that is now in use by somebody else.
 
This can't happen because memblock_double_array() uses kmalloc() as soon as
slab_is_available(), so this sentence is misleading.

> Avoid those issues by making sure that memblock_discard points
> memblock.reserved.regions back at the static buffer.
>
> If memblock_discard is called while there is still memory
> in the memblock.reserved type, that will print a warning
> in memblock_remove_region.
> 
> Signed-off-by: Rik van Riel <riel@surriel.com>
> ---
>  mm/memblock.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 3feafea06ab2..068289a46903 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -374,6 +374,10 @@ void __init memblock_discard(void)
>  			kfree(memblock.reserved.regions);
>  		else
>  			memblock_free_late(addr, size);
> +		/* Reset to prevent UAF from stray frees. */
> +		memblock.reserved.regions = memblock_reserved_init_regions;
> +		memblock.reserved.cnt = 1;
> +		memblock_remove_region(&memblock.reserved, 0);
>  	}
>  
>  	if (memblock.memory.regions != memblock_memory_init_regions) {
> -- 
> 2.34.1
> 
>
Rik van Riel July 20, 2023, 1:15 p.m. UTC | #2 
On Thu, 2023-07-20 at 08:00 +0300, Mike Rapoport wrote:
> Hi Ric,
> 
> On Wed, Jul 19, 2023 at 03:41:37PM -0400, Rik van Riel wrote:
> > The memblock_discard function frees the memblock.reserved.regions
> > array, which is good.
> > 
> > However, if a subsequent memblock_free (or memblock_phys_free)
> > comes
> > in later, from for example ima_free_kexec_buffer, that will result
> > in
> > a use after free bug in memblock_isolate_range.
> 
> The use of memblock_phys_free() in ima_free_kexec_buffer() is
> entirely
> bogus and must be fixed. It should be memblock_free_late() there.
> 
I'll send in a patch for that code, then. Thank you!

> > When running a kernel with CONFIG_KASAN enabled, this will cause a
> > kernel panic very early in boot. Without CONFIG_KASAN, there is
> > a chance that memblock_isolate_range might scribble on memory
> > that is now in use by somebody else.
>  
> This can't happen because memblock_double_array() uses kmalloc() as
> soon as
> slab_is_available(), so this sentence is misleading.

memblock_discard() freed the array, but did not change
the pointer. That resulted in memblock_isolate_range()
following a stale pointer.

There was no call to memblock_double_array() in the
final call that crashed. I checked that by booting
with memblock=debug.

kind regards,

Rik van Riel
Mike Rapoport July 20, 2023, 4:25 p.m. UTC | #3 
On Wed, Jul 19, 2023 at 03:41:37PM -0400, Rik van Riel wrote:
> The memblock_discard function frees the memblock.reserved.regions
> array, which is good.
> 
> However, if a subsequent memblock_free (or memblock_phys_free) comes
> in later, from for example ima_free_kexec_buffer, that will result in
> a use after free bug in memblock_isolate_range.
> 
> When running a kernel with CONFIG_KASAN enabled, this will cause a
> kernel panic very early in boot. Without CONFIG_KASAN, there is
> a chance that memblock_isolate_range might scribble on memory
> that is now in use by somebody else.
> 
> Avoid those issues by making sure that memblock_discard points
> memblock.reserved.regions back at the static buffer.
> 
> If memblock_discard is called while there is still memory
> in the memblock.reserved type, that will print a warning
> in memblock_remove_region.

I'm going to apply this with the last paragraph rewritten as 

If memblock_free is called after memblock memory is discarded, that will
print a warning in memblock_remove_region.
 
> Signed-off-by: Rik van Riel <riel@surriel.com>
> ---
>  mm/memblock.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 3feafea06ab2..068289a46903 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -374,6 +374,10 @@ void __init memblock_discard(void)
>  			kfree(memblock.reserved.regions);
>  		else
>  			memblock_free_late(addr, size);
> +		/* Reset to prevent UAF from stray frees. */
> +		memblock.reserved.regions = memblock_reserved_init_regions;
> +		memblock.reserved.cnt = 1;
> +		memblock_remove_region(&memblock.reserved, 0);
>  	}
>  
>  	if (memblock.memory.regions != memblock_memory_init_regions) {
> -- 
> 2.34.1
> 
>
Guenter Roeck July 28, 2023, 4:09 p.m. UTC | #4 
Hi,

On Wed, Jul 19, 2023 at 03:41:37PM -0400, Rik van Riel wrote:
> The memblock_discard function frees the memblock.reserved.regions
> array, which is good.
> 
> However, if a subsequent memblock_free (or memblock_phys_free) comes
> in later, from for example ima_free_kexec_buffer, that will result in
> a use after free bug in memblock_isolate_range.
> 
> When running a kernel with CONFIG_KASAN enabled, this will cause a
> kernel panic very early in boot. Without CONFIG_KASAN, there is
> a chance that memblock_isolate_range might scribble on memory
> that is now in use by somebody else.
> 
> Avoid those issues by making sure that memblock_discard points
> memblock.reserved.regions back at the static buffer.
> 
> If memblock_discard is called while there is still memory
> in the memblock.reserved type, that will print a warning
> in memblock_remove_region.
> 
> Signed-off-by: Rik van Riel <riel@surriel.com>

This patch results in the following WARNING backtrace when booting sparc
or sparc64 images in qemu. Bisect log is attached.

sparc:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at mm/memblock.c:352 memblock_remove_region+0x80/0xb0
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 6.5.0-rc3-00350-gb917f578dc45 #1
[f0022af8 : __warn+0x9c/0xe4 ]
[f0022b84 : warn_slowpath_fmt+0x44/0x54 ]
[f07a16a0 : memblock_remove_region+0x80/0xb0 ]
[f0787240 : memblock_discard+0x84/0x100 ]
[f0784e00 : page_alloc_init_late+0xc/0x5c ]
[f07782fc : kernel_init_freeable+0xb8/0x208 ]
[f05ad41c : kernel_init+0x10/0x120 ]
[f0007ed0 : ret_from_kernel_thread+0xc/0x38 ]
[00000000 : 0x0 ]
---[ end trace 0000000000000000 ]---

sparc64:

[    1.876345] ------------[ cut here ]------------
[    1.876852] WARNING: CPU: 0 PID: 1 at mm/memblock.c:352 memblock_remove_region+0x78/0xb4
[    1.877912] Modules linked in:
[    1.880783] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.5.0-rc3+ #1
[    1.881473] Call Trace:
[    1.881935] [<0000000000467c30>] __warn+0xf0/0x1a0
[    1.882515] [<0000000000467d98>] warn_slowpath_fmt+0xb8/0x100
[    1.883128] [<0000000001b8d83c>] memblock_remove_region+0x78/0xb4
[    1.883564] [<0000000001b6df50>] memblock_discard+0x88/0x108
[    1.883987] [<0000000001b6abc0>] page_alloc_init_late+0xc/0x94
[    1.884594] [<0000000001b56e44>] kernel_init_freeable+0xcc/0x228
[    1.885181] [<0000000000f673c4>] kernel_init+0x18/0x134
[    1.885612] [<00000000004060e8>] ret_from_fork+0x1c/0x2c
[    1.886035] [<0000000000000000>] 0x0
[    1.886697] irq event stamp: 1013
[    1.887293] hardirqs last  enabled at (1021): [<00000000004e1534>] __up_console_sem+0x74/0xa0
[    1.887786] hardirqs last disabled at (1028): [<00000000004e1510>] __up_console_sem+0x50/0xa0
[    1.888273] softirqs last  enabled at (1004): [<0000000000f76ae4>] __do_softirq+0x4c4/0x5a0
[    1.888760] softirqs last disabled at (997): [<000000000042b94c>] do_softirq_own_stack+0x2c/0x40
[    1.889610] ---[ end trace 0000000000000000 ]---

Guenter

---
# bad: [57012c57536f8814dec92e74197ee96c3498d24e] Merge tag 'net-6.5-rc4' of git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net
# good: [18b44bc5a67275641fb26f2c54ba7eef80ac5950] ovl: Always reevaluate the file signature for IMA
git bisect start 'HEAD' '18b44bc5a672'
# good: [6c58c8816abb7b93b21fa3b1d0c1726402e5e568] net/sched: mqprio: Add length check for TCA_MQPRIO_{MAX/MIN}_RATE64
git bisect good 6c58c8816abb7b93b21fa3b1d0c1726402e5e568
# bad: [379e66711b33f9fdc0513daee6cf3dd8d2f6f435] Merge tag 'fixes-2023-07-27' of git://git.kernel.org/pub/scm/linux/kernel/git/rppt/memblock
git bisect bad 379e66711b33f9fdc0513daee6cf3dd8d2f6f435
# good: [c21733754cd6ecbca346f2adf9b17d4cfa50504f] platform/x86: huawei-wmi: Silence ambient light sensor
git bisect good c21733754cd6ecbca346f2adf9b17d4cfa50504f
# good: [536bb492d39bb6c080c92f31e8a55fe9934f452b] ksmbd: fix out of bounds in init_smb2_rsp_hdr()
git bisect good 536bb492d39bb6c080c92f31e8a55fe9934f452b
# good: [0a8db05b571ad5b8d5c8774a004c0424260a90bd] Merge tag 'platform-drivers-x86-v6.5-3' of git://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86
git bisect good 0a8db05b571ad5b8d5c8774a004c0424260a90bd
# bad: [9e46e4dcd9d6cd88342b028dbfa5f4fb7483d39c] mm,memblock: reset memblock.reserved to system init state to prevent UAF
git bisect bad 9e46e4dcd9d6cd88342b028dbfa5f4fb7483d39c
# first bad commit: [9e46e4dcd9d6cd88342b028dbfa5f4fb7483d39c] mm,memblock: reset memblock.reserved to system init state to prevent UAF
Guenter Roeck July 28, 2023, 4:36 p.m. UTC | #5 
On Fri, Jul 28, 2023 at 09:09:09AM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Wed, Jul 19, 2023 at 03:41:37PM -0400, Rik van Riel wrote:
> > The memblock_discard function frees the memblock.reserved.regions
> > array, which is good.
> > 
> > However, if a subsequent memblock_free (or memblock_phys_free) comes
> > in later, from for example ima_free_kexec_buffer, that will result in
> > a use after free bug in memblock_isolate_range.
> > 
> > When running a kernel with CONFIG_KASAN enabled, this will cause a
> > kernel panic very early in boot. Without CONFIG_KASAN, there is
> > a chance that memblock_isolate_range might scribble on memory
> > that is now in use by somebody else.
> > 
> > Avoid those issues by making sure that memblock_discard points
> > memblock.reserved.regions back at the static buffer.
> > 
> > If memblock_discard is called while there is still memory
> > in the memblock.reserved type, that will print a warning
> > in memblock_remove_region.
> > 
> > Signed-off-by: Rik van Riel <riel@surriel.com>
> 
> This patch results in the following WARNING backtrace when booting sparc
> or sparc64 images in qemu. Bisect log is attached.
> 

Follow-up: On sparc64, this patch also results in the following backtrace.

[    2.931808] BUG: scheduling while atomic: swapper/0/1/0x00000002
[    2.932865] no locks held by swapper/0/1.
[    2.933722] Modules linked in:
[    2.934627] CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W          6.5.0-rc3+ #1
[    2.935604] Call Trace:
[    2.936315] [<00000000004a0610>] __schedule_bug+0x70/0x80
[    2.937174] [<0000000000f68f50>] switch_to_pc+0x598/0x8e8
[    2.937999] [<0000000000f69300>] schedule+0x60/0xe0
[    2.938811] [<0000000000f72d2c>] schedule_timeout+0x10c/0x1c0
[    2.939668] [<0000000000f69be0>] __wait_for_common+0xa0/0x1a0
[    2.940510] [<0000000000f69d98>] wait_for_completion_killable+0x18/0x40
[    2.941402] [<0000000000494dec>] __kthread_create_on_node+0xac/0x120
[    2.942259] [<0000000000494e80>] kthread_create_on_node+0x20/0x40
[    2.943023] [<0000000001b81348>] devtmpfs_init+0xb4/0x140
[    2.943777] [<0000000001b81068>] driver_init+0x10/0x60
[    2.944528] [<0000000001b56e4c>] kernel_init_freeable+0xd4/0x228
[    2.945300] [<0000000000f67404>] kernel_init+0x18/0x134
[    2.946026] [<00000000004060e8>] ret_from_fork+0x1c/0x2c
[    2.946757] [<0000000000000000>] 0x0
[    2.959537] devtmpfs: initialized

While that seemed unlikely (and I don't claim to understand it), I ran
bisect separately and confirmed that both tracebacks are gone after
reverting this patch.

Guenter
diff mbox series

Patch

diff --git a/mm/memblock.c b/mm/memblock.c
index 3feafea06ab2..068289a46903 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -374,6 +374,10 @@  void __init memblock_discard(void)
 			kfree(memblock.reserved.regions);
 		else
 			memblock_free_late(addr, size);
+		/* Reset to prevent UAF from stray frees. */
+		memblock.reserved.regions = memblock_reserved_init_regions;
+		memblock.reserved.cnt = 1;
+		memblock_remove_region(&memblock.reserved, 0);
 	}
 
 	if (memblock.memory.regions != memblock_memory_init_regions) {