| Message ID | 20230719154137.732d8525@imladris.surriel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm,memblock: reset memblock.reserved to system init state to prevent UAF | expand |
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 > >
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
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 > >
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
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 --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) {
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(+)