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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
>
>
--
Sincerely yours,
Mike.
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
--
All Rights Reversed.
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 <[email protected]>
> ---
> 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
>
>
--
Sincerely yours,
Mike.
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 <[email protected]>
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
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 <[email protected]>
>
> 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