* [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently
@ 2022-01-28 9:03 h00486469
2022-02-04 12:36 ` Catalin Marinas
0 siblings, 1 reply; 3+ messages in thread
From: h00486469 @ 2022-01-28 9:03 UTC (permalink / raw)
To: catalin.marinas, will, punit.agrawal, peterz, linux-kernel
Cc: hewenliang4, hejingxian
From: hewenliang <hewenliang4@huawei.com>
SAN reports an issue of slab-out-of-bounds in emulation_proc_handler
when we try to read/write the interfaces in /proc/sys/abi concurrently.
So we need to add emulation_proc_lock to protect table->data and insn
from data corruption in emulation_proc_handler.
The stack is follows:
Call trace:
dump_backtrace+0x0/0x310
show_stack+0x28/0x38
dump_stack+0xec/0x15c
print_address_description+0x68/0x2d0
kasan_report+0x130/0x2f0
__asan_load4+0x88/0xb0
emulation_proc_handler+0x58/0x158
proc_sys_call_handler+0x1dc/0x228
proc_sys_read+0x44/0x58
__vfs_read+0xe0/0x320
vfs_read+0xbc/0x1c0
__arm64_sys_read+0x50/0x60
el0_svc_common+0xc8/0x2b8
el0_svc_handler+0xf8/0x160
el0_svc+0x10/0x218
Allocated by task 1:
kasan_kmalloc+0xe0/0x190
kmem_cache_alloc_trace+0x18c/0x418
register_insn_emulation+0x4c/0x2b0
armv8_deprecated_init+0x40/0x108
do_one_initcall+0xb4/0x508
kernel_init_freeable+0x7d0/0x8e0
kernel_init+0x20/0x1a8
ret_from_fork+0x10/0x18
Mmeory state around the buggy address:
>ffff8026dacf0b00: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
Fixes: 587064b610c7 ("arm64: Add framework for legacy instruction emulation")
Signed-off-by: hewenliang <hewenliang4@huawei.com>
Signed-off-by: hejingxian <hejingxian@huawei.com>
Signed-off-by: fulin <fulin@huawei.com>
---
arch/arm64/kernel/armv8_deprecated.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
index 6875a16b09d2..d2ac483b0dd8 100644
--- a/arch/arm64/kernel/armv8_deprecated.c
+++ b/arch/arm64/kernel/armv8_deprecated.c
@@ -59,6 +59,7 @@ struct insn_emulation {
static LIST_HEAD(insn_emulation);
static int nr_insn_emulated __initdata;
static DEFINE_RAW_SPINLOCK(insn_emulation_lock);
+static DEFINE_MUTEX(emulation_proc_lock);
static void register_emulation_hooks(struct insn_emulation_ops *ops)
{
@@ -207,9 +208,12 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
loff_t *ppos)
{
int ret = 0;
- struct insn_emulation *insn = (struct insn_emulation *) table->data;
- enum insn_emulation_mode prev_mode = insn->current_mode;
+ struct insn_emulation *insn;
+ enum insn_emulation_mode prev_mode;
+ mutex_lock(&emulation_proc_lock);
+ insn = (struct insn_emulation *) table->data;
+ prev_mode = insn->current_mode;
table->data = &insn->current_mode;
ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
@@ -224,6 +228,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
}
ret:
table->data = insn;
+ mutex_unlock(&emulation_proc_lock);
return ret;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently
2022-01-28 9:03 [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently h00486469
@ 2022-02-04 12:36 ` Catalin Marinas
0 siblings, 0 replies; 3+ messages in thread
From: Catalin Marinas @ 2022-02-04 12:36 UTC (permalink / raw)
To: h00486469
Cc: will, Punit Agrawal, peterz, linux-kernel, hejingxian, linux-arm-kernel
I corrected Punit's email address. Also please cc
linux-arm-kernel@lists.infradead.org in the future (you can use
scripts/get_maintainer.pl to give you a hint on who to cc).
On Fri, Jan 28, 2022 at 05:03:24PM +0800, h00486469 wrote:
> From: hewenliang <hewenliang4@huawei.com>
>
> SAN reports an issue of slab-out-of-bounds in emulation_proc_handler
> when we try to read/write the interfaces in /proc/sys/abi concurrently.
> So we need to add emulation_proc_lock to protect table->data and insn
> from data corruption in emulation_proc_handler.
>
> The stack is follows:
> Call trace:
> dump_backtrace+0x0/0x310
> show_stack+0x28/0x38
> dump_stack+0xec/0x15c
> print_address_description+0x68/0x2d0
> kasan_report+0x130/0x2f0
> __asan_load4+0x88/0xb0
> emulation_proc_handler+0x58/0x158
> proc_sys_call_handler+0x1dc/0x228
> proc_sys_read+0x44/0x58
> __vfs_read+0xe0/0x320
> vfs_read+0xbc/0x1c0
> __arm64_sys_read+0x50/0x60
> el0_svc_common+0xc8/0x2b8
> el0_svc_handler+0xf8/0x160
> el0_svc+0x10/0x218
>
> Allocated by task 1:
> kasan_kmalloc+0xe0/0x190
> kmem_cache_alloc_trace+0x18c/0x418
> register_insn_emulation+0x4c/0x2b0
> armv8_deprecated_init+0x40/0x108
> do_one_initcall+0xb4/0x508
> kernel_init_freeable+0x7d0/0x8e0
> kernel_init+0x20/0x1a8
> ret_from_fork+0x10/0x18
>
> Mmeory state around the buggy address:
> >ffff8026dacf0b00: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
>
> Fixes: 587064b610c7 ("arm64: Add framework for legacy instruction emulation")
> Signed-off-by: hewenliang <hewenliang4@huawei.com>
> Signed-off-by: hejingxian <hejingxian@huawei.com>
> Signed-off-by: fulin <fulin@huawei.com>
> ---
> arch/arm64/kernel/armv8_deprecated.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index 6875a16b09d2..d2ac483b0dd8 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -59,6 +59,7 @@ struct insn_emulation {
> static LIST_HEAD(insn_emulation);
> static int nr_insn_emulated __initdata;
> static DEFINE_RAW_SPINLOCK(insn_emulation_lock);
> +static DEFINE_MUTEX(emulation_proc_lock);
>
> static void register_emulation_hooks(struct insn_emulation_ops *ops)
> {
> @@ -207,9 +208,12 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> loff_t *ppos)
> {
> int ret = 0;
> - struct insn_emulation *insn = (struct insn_emulation *) table->data;
> - enum insn_emulation_mode prev_mode = insn->current_mode;
> + struct insn_emulation *insn;
> + enum insn_emulation_mode prev_mode;
>
> + mutex_lock(&emulation_proc_lock);
> + insn = (struct insn_emulation *) table->data;
> + prev_mode = insn->current_mode;
> table->data = &insn->current_mode;
> ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
It looks like we update table->data to something that's not the original
insn pointer just to be able to call proc_dointvec_minmax(). On a
concurrent call, we'd get the wrong pointer hence the ASAN warning. I'd
rather keep the table->data as &insn->current_mode and use
container_of() to retrieve the insn pointer. We probably still need a
mutex to protect against the current_mode update and the registration of
the emulation hooks but not for retrieving insn as table->data is no
longer changing.
>
> @@ -224,6 +228,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> }
> ret:
> table->data = insn;
> + mutex_unlock(&emulation_proc_lock);
> return ret;
> }
>
> --
> 2.27.0
--
Catalin
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently
@ 2022-02-04 12:36 ` Catalin Marinas
0 siblings, 0 replies; 3+ messages in thread
From: Catalin Marinas @ 2022-02-04 12:36 UTC (permalink / raw)
To: h00486469
Cc: will, Punit Agrawal, peterz, linux-kernel, hejingxian, linux-arm-kernel
I corrected Punit's email address. Also please cc
linux-arm-kernel@lists.infradead.org in the future (you can use
scripts/get_maintainer.pl to give you a hint on who to cc).
On Fri, Jan 28, 2022 at 05:03:24PM +0800, h00486469 wrote:
> From: hewenliang <hewenliang4@huawei.com>
>
> SAN reports an issue of slab-out-of-bounds in emulation_proc_handler
> when we try to read/write the interfaces in /proc/sys/abi concurrently.
> So we need to add emulation_proc_lock to protect table->data and insn
> from data corruption in emulation_proc_handler.
>
> The stack is follows:
> Call trace:
> dump_backtrace+0x0/0x310
> show_stack+0x28/0x38
> dump_stack+0xec/0x15c
> print_address_description+0x68/0x2d0
> kasan_report+0x130/0x2f0
> __asan_load4+0x88/0xb0
> emulation_proc_handler+0x58/0x158
> proc_sys_call_handler+0x1dc/0x228
> proc_sys_read+0x44/0x58
> __vfs_read+0xe0/0x320
> vfs_read+0xbc/0x1c0
> __arm64_sys_read+0x50/0x60
> el0_svc_common+0xc8/0x2b8
> el0_svc_handler+0xf8/0x160
> el0_svc+0x10/0x218
>
> Allocated by task 1:
> kasan_kmalloc+0xe0/0x190
> kmem_cache_alloc_trace+0x18c/0x418
> register_insn_emulation+0x4c/0x2b0
> armv8_deprecated_init+0x40/0x108
> do_one_initcall+0xb4/0x508
> kernel_init_freeable+0x7d0/0x8e0
> kernel_init+0x20/0x1a8
> ret_from_fork+0x10/0x18
>
> Mmeory state around the buggy address:
> >ffff8026dacf0b00: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
>
> Fixes: 587064b610c7 ("arm64: Add framework for legacy instruction emulation")
> Signed-off-by: hewenliang <hewenliang4@huawei.com>
> Signed-off-by: hejingxian <hejingxian@huawei.com>
> Signed-off-by: fulin <fulin@huawei.com>
> ---
> arch/arm64/kernel/armv8_deprecated.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kernel/armv8_deprecated.c b/arch/arm64/kernel/armv8_deprecated.c
> index 6875a16b09d2..d2ac483b0dd8 100644
> --- a/arch/arm64/kernel/armv8_deprecated.c
> +++ b/arch/arm64/kernel/armv8_deprecated.c
> @@ -59,6 +59,7 @@ struct insn_emulation {
> static LIST_HEAD(insn_emulation);
> static int nr_insn_emulated __initdata;
> static DEFINE_RAW_SPINLOCK(insn_emulation_lock);
> +static DEFINE_MUTEX(emulation_proc_lock);
>
> static void register_emulation_hooks(struct insn_emulation_ops *ops)
> {
> @@ -207,9 +208,12 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> loff_t *ppos)
> {
> int ret = 0;
> - struct insn_emulation *insn = (struct insn_emulation *) table->data;
> - enum insn_emulation_mode prev_mode = insn->current_mode;
> + struct insn_emulation *insn;
> + enum insn_emulation_mode prev_mode;
>
> + mutex_lock(&emulation_proc_lock);
> + insn = (struct insn_emulation *) table->data;
> + prev_mode = insn->current_mode;
> table->data = &insn->current_mode;
> ret = proc_dointvec_minmax(table, write, buffer, lenp, ppos);
It looks like we update table->data to something that's not the original
insn pointer just to be able to call proc_dointvec_minmax(). On a
concurrent call, we'd get the wrong pointer hence the ASAN warning. I'd
rather keep the table->data as &insn->current_mode and use
container_of() to retrieve the insn pointer. We probably still need a
mutex to protect against the current_mode update and the registration of
the emulation hooks but not for retrieving insn as table->data is no
longer changing.
>
> @@ -224,6 +228,7 @@ static int emulation_proc_handler(struct ctl_table *table, int write,
> }
> ret:
> table->data = insn;
> + mutex_unlock(&emulation_proc_lock);
> return ret;
> }
>
> --
> 2.27.0
--
Catalin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-02-04 12:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-28 9:03 [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently h00486469
2022-02-04 12:36 ` Catalin Marinas
2022-02-04 12:36 ` Catalin Marinas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.