Messages in this thread | | | From | Dmitry Vyukov <> | Date | Wed, 3 Feb 2021 20:35:44 +0100 | Subject | Re: [PATCH 1/1] iscsi_ibft: KASAN false positive failure occurs in ibft_init() |
| |
On Wed, Feb 3, 2021 at 8:29 PM Konrad Rzeszutek Wilk <konrad@darnok.org> wrote: > > Hey Dmitry, Rafael, George, please see below.. > > On Wed, Jan 27, 2021 at 10:10:07PM +0100, Dmitry Vyukov wrote: > > On Wed, Jan 27, 2021 at 9:01 PM George Kennedy > > <george.kennedy@oracle.com> wrote: > > > > > > Hi Dmitry, > > > > > > On 1/27/2021 1:48 PM, Dmitry Vyukov wrote: > > > > > > On Wed, Jan 27, 2021 at 7:44 PM Konrad Rzeszutek Wilk > > > <konrad.wilk@oracle.com> wrote: > > > > > > On Tue, Jan 26, 2021 at 01:03:21PM -0500, George Kennedy wrote: > > > > > > During boot of kernel with CONFIG_KASAN the following KASAN false > > > positive failure will occur when ibft_init() reads the > > > ACPI iBFT table: BUG: KASAN: use-after-free in ibft_init > > > > > > The ACPI iBFT table is not allocated, and the iscsi driver uses > > > a pointer to it to calculate checksum, etc. KASAN complains > > > about this pointer with use-after-free, which this is not. > > > > > > Andrey, Alexander, Dmitry, > > > > > > I think this is the right way for this, but was wondering if you have > > > other suggestions? > > > > > > Thanks! > > > > > > Hi George, Konrad, > > > > > > Please provide a sample KASAN report and kernel version to match line numbers. > > > > > > 5.4.17-2102.200.0.0.20210106_0000 > > > > > > [ 24.413536] iBFT detected. > > > [ 24.414074] > > > ================================================================== > > > [ 24.407342] BUG: KASAN: use-after-free in ibft_init+0x134/0xb8b > > > [ 24.407342] Read of size 4 at addr ffff8880be452004 by task swapper/0/1 > > > [ 24.407342] > > > [ 24.407342] CPU: 1 PID: 1 Comm: swapper/0 Not tainted 5.4.17-2102.200.0.0.20210106_0000.syzk #1 > > > [ 24.407342] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > > > [ 24.407342] Call Trace: > > > [ 24.407342] dump_stack+0xd4/0x119 > > > [ 24.407342] ? ibft_init+0x134/0xb8b > > > [ 24.407342] print_address_description.constprop.6+0x20/0x220 > > > [ 24.407342] ? ibft_init+0x134/0xb8b > > > [ 24.407342] ? ibft_init+0x134/0xb8b > > > [ 24.407342] __kasan_report.cold.9+0x37/0x77 > > > [ 24.407342] ? ibft_init+0x134/0xb8b > > > [ 24.407342] kasan_report+0x14/0x1b > > > [ 24.407342] __asan_report_load_n_noabort+0xf/0x11 > > > [ 24.407342] ibft_init+0x134/0xb8b > > > [ 24.407342] ? dmi_sysfs_init+0x1a5/0x1a5 > > > [ 24.407342] ? dmi_walk+0x72/0x89 > > > [ 24.407342] ? ibft_check_initiator_for+0x159/0x159 > > > [ 24.407342] ? rvt_init_port+0x110/0x101 > > > [ 24.407342] ? ibft_check_initiator_for+0x159/0x159 > > > [ 24.407342] do_one_initcall+0xc3/0x44d > > > [ 24.407342] ? perf_trace_initcall_level+0x410/0x405 > > > [ 24.407342] kernel_init_freeable+0x551/0x673 > > > [ 24.407342] ? start_kernel+0x94b/0x94b > > > [ 24.407342] ? __sanitizer_cov_trace_const_cmp1+0x1a/0x1c > > > [ 24.407342] ? __kasan_check_write+0x14/0x16 > > > [ 24.407342] ? rest_init+0xe6/0xe6 > > > [ 24.407342] kernel_init+0x16/0x1bd > > > [ 24.407342] ? rest_init+0xe6/0xe6 > > > [ 24.407342] ret_from_fork+0x2b/0x36 > > > [ 24.407342] > > > [ 24.407342] The buggy address belongs to the page: > > > [ 24.407342] page:ffffea0002f91480 refcount:0 mapcount:0 mapping:0000000000000000 index:0x1 > > > [ 24.407342] flags: 0xfffffc0000000() > > > [ 24.407342] raw: 000fffffc0000000 ffffea0002fca588 ffffea0002fb1a88 0000000000000000 > > > [ 24.407342] raw: 0000000000000001 0000000000000000 00000000ffffffff 0000000000000000 > > > [ 24.407342] page dumped because: kasan: bad access detected > > > [ 24.407342] > > > [ 24.407342] Memory state around the buggy address: > > > [ 24.407342] ffff8880be451f00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > [ 24.407342] ffff8880be451f80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > [ 24.407342] >ffff8880be452000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > [ 24.407342] ^ > > > [ 24.407342] ffff8880be452080: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > [ 24.407342] ffff8880be452100: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff > > > [ 24.407342] > > > ================================================================== > > > [ 24.407342] Disabling lock debugging due to kernel taint > > > [ 24.451021] Kernel panic - not syncing: panic_on_warn set ... > > > [ 24.452002] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G B 5.4.17-2102.200.0.0.20210106_0000.syzk #1 > > > [ 24.452002] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 0.0.0 02/06/2015 > > > [ 24.452002] Call Trace: > > > [ 24.452002] dump_stack+0xd4/0x119 > > > [ 24.452002] ? ibft_init+0x102/0xb8b > > > [ 24.452002] panic+0x28f/0x6e0 > > > [ 24.452002] ? __warn_printk+0xe0/0xe0 > > > [ 24.452002] ? ibft_init+0x134/0xb8b > > > [ 24.452002] ? add_taint+0x68/0xb3 > > > [ 24.452002] ? add_taint+0x68/0xb3 > > > [ 24.452002] ? ibft_init+0x134/0xb8b > > > [ 24.452002] ? ibft_init+0x134/0xb8b > > > [ 24.452002] end_report+0x4c/0x54 > > > [ 24.452002] __kasan_report.cold.9+0x55/0x77 > > > [ 24.452002] ? ibft_init+0x134/0xb8b > > > [ 24.452002] kasan_report+0x14/0x1b > > > [ 24.452002] __asan_report_load_n_noabort+0xf/0x11 > > > [ 24.452002] ibft_init+0x134/0xb8b > > > [ 24.452002] ? dmi_sysfs_init+0x1a5/0x1a5 > > > [ 24.452002] ? dmi_walk+0x72/0x89 > > > [ 24.452002] ? ibft_check_initiator_for+0x159/0x159 > > > [ 24.452002] ? rvt_init_port+0x110/0x101 > > > [ 24.452002] ? ibft_check_initiator_for+0x159/0x159 > > > [ 24.452002] do_one_initcall+0xc3/0x44d > > > [ 24.452002] ? perf_trace_initcall_level+0x410/0x405 > > > [ 24.452002] kernel_init_freeable+0x551/0x673 > > > [ 24.452002] ? start_kernel+0x94b/0x94b > > > [ 24.452002] ? __sanitizer_cov_trace_const_cmp1+0x1a/0x1c > > > [ 24.452002] ? __kasan_check_write+0x14/0x16 > > > [ 24.452002] ? rest_init+0xe6/0xe6 > > > [ 24.452002] kernel_init+0x16/0x1bd > > > [ 24.452002] ? rest_init+0xe6/0xe6 > > > [ 24.452002] ret_from_fork+0x2b/0x36 > > > [ 24.452002] Dumping ftrace buffer: > > > [ 24.452002] --------------------------------- > > > [ 24.452002] swapper/-1 1.... 24564337us : rdmaip_init: 2924: rdmaip_init: Active Bonding is DISABLED > > > [ 24.452002] --------------------------------- > > > [ 24.452002] Kernel Offset: disabled > > > [ 24.452002] Rebooting in 1 seconds.. > > > > > > Why does KASAN think the address is freed? For that to happen that > > > memory should have been freed. I don't remember any similar false > > > positives from KASAN, so this looks a bit suspicious. > > > > > > I'm not sure why KASAN thinks the address is freed. There are other modules where KASAN/KCOV is disabled on boot. > > > Could this be for a similar reason? > > > > Most of these files are disabled because they cause recursion in > > instrumentation, or execute too early in bootstrap process (before > > kasan_init). > > > > Somehow the table pointer in ibft_init points to a freed page. I > > tracked it down to here: > > https://elixir.bootlin.com/linux/v5.4.17/source/drivers/acpi/acpica/tbutils.c#L399 > > but I can't find where this table_desc->pointer comes from. Perhaps it > > It is what the BIOS generated. It usually points to some memory > location in right under 4GB and the BIOS stashes the DSDT, iBFT, and > other tables in there. > > > uses some allocation method that's not supported by KASAN? However, > > it's the only such case that I've seen, so it's a bit weird. Could it > > use something like memblock_alloc? Or maybe that page was in fact > > freed?... Too bad KASAN does not print free stack for pages, maybe > > it's not too hard to do if CONFIG_PAGE_OWNER is enabled... > > Hm, there is a comment in the acpi_get_table speaking about the > requirement of having a acpi_put_table and: > > > * DESCRIPTION: Finds and verifies an ACPI table. Table must be in the > * RSDT/XSDT. > * Note that an early stage acpi_get_table() call must be paired > * with an early stage acpi_put_table() call. otherwise the table > * pointer mapped by the early stage mapping implementation may be > * erroneously unmapped by the late stage unmapping implementation > * in an acpi_put_table() invoked during the late stage. > * > > Which would imply that I should use acpi_put_table in the error path > (see below a patch), but also copy the structure instead of depending > on ACPI keeping it mapped for me. I think.
Hi Konrad,
Thanks for looking into this. If ACPI unmaps this page, that would perfectly explain the KASAN report.
George, does this patch eliminate the KASAN report for you?
> CC-ing Rafeal. > > > From c37da50fdfc62cd4f7b23562b55661478c90a17d Mon Sep 17 00:00:00 2001 > From: Konrad Rzeszutek Wilk <konrad@darnok.org> > Date: Tue, 2 Feb 2021 17:28:28 +0000 > Subject: [PATCH] ibft: Put ibft_addr back > > Signed-off-by: Konrad Rzeszutek Wilk <konrad@darnok.org> > --- > drivers/firmware/iscsi_ibft.c | 19 +++++++++++++------ > 1 file changed, 13 insertions(+), 6 deletions(-) > > diff --git a/drivers/firmware/iscsi_ibft.c b/drivers/firmware/iscsi_ibft.c > index 7127a04..2a1a033 100644 > --- a/drivers/firmware/iscsi_ibft.c > +++ b/drivers/firmware/iscsi_ibft.c > @@ -811,6 +811,10 @@ static void ibft_cleanup(void) > ibft_unregister(); > iscsi_boot_destroy_kset(boot_kset); > } > + if (ibft_addr) { > + acpi_put_table((struct acpi_table_header *)ibft_addr); > + ibft_addr = NULL; > + } > } > > static void __exit ibft_exit(void) > @@ -835,13 +839,15 @@ static void __init acpi_find_ibft_region(void) > { > int i; > struct acpi_table_header *table = NULL; > + acpi_status status; > > if (acpi_disabled) > return; > > for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++) { > - acpi_get_table(ibft_signs[i].sign, 0, &table); > - ibft_addr = (struct acpi_table_ibft *)table; > + status = acpi_get_table(ibft_signs[i].sign, 0, &table); > + if (ACPI_SUCCESS(status)) > + ibft_addr = (struct acpi_table_ibft *)table; > } > } > #else > @@ -870,12 +876,13 @@ static int __init ibft_init(void) > > rc = ibft_check_device(); > if (rc) > - return rc; > + goto out_free; > > boot_kset = iscsi_boot_create_kset("ibft"); > - if (!boot_kset) > - return -ENOMEM; > - > + if (!boot_kset) { > + rc = -ENOMEM; > + goto out_free; > + } > /* Scan the IBFT for data and register the kobjects. */ > rc = ibft_register_kobjects(ibft_addr); > if (rc) > -- > 1.8.3.1 >
| |