From: Catalin Marinas <catalin.marinas@arm.com> To: h00486469 <hewenliang4@huawei.com> Cc: will@kernel.org, Punit Agrawal <punitagrawal@gmail.com>, peterz@infradead.org, linux-kernel@vger.kernel.org, hejingxian@huawei.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently Date: Fri, 4 Feb 2022 12:36:22 +0000 [thread overview] Message-ID: <Yf0dxon1d07rzxZH@arm.com> (raw) In-Reply-To: <20220128090324.2727688-1-hewenliang4@huawei.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com> To: h00486469 <hewenliang4@huawei.com> Cc: will@kernel.org, Punit Agrawal <punitagrawal@gmail.com>, peterz@infradead.org, linux-kernel@vger.kernel.org, hejingxian@huawei.com, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] arm64: fix slab-out-of-bounds in emulation_proc_handler when accessing concurrently Date: Fri, 4 Feb 2022 12:36:22 +0000 [thread overview] Message-ID: <Yf0dxon1d07rzxZH@arm.com> (raw) In-Reply-To: <20220128090324.2727688-1-hewenliang4@huawei.com> 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
next prev parent reply other threads:[~2022-02-04 12:36 UTC|newest] Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top 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 [this message] 2022-02-04 12:36 ` Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yf0dxon1d07rzxZH@arm.com \ --to=catalin.marinas@arm.com \ --cc=hejingxian@huawei.com \ --cc=hewenliang4@huawei.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=peterz@infradead.org \ --cc=punitagrawal@gmail.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.