Re: slub freelist issue / BUG: unable to handle page fault for address: 000000003ffe0018

[Kees Cook]

On Fri, Jun 05, 2020 at 04:44:51PM +0200, Vegard Nossum wrote:
> On 2020-06-05 16:08, Vlastimil Babka wrote:
> > On 6/5/20 3:12 PM, Rafael J. Wysocki wrote:
> > > On Fri, Jun 5, 2020 at 2:48 PM Vegard Nossum <vegard.nossum@xxxxxxxxxx> wrote:
> > > > 
> > > > On 2020-06-05 11:36, Vegard Nossum wrote:
> > > > > 
> > > > > On 2020-06-05 11:11, Vlastimil Babka wrote:
> > > > > > So, with Kees' patch reverted, booting with slub_debug=F (or even more
> > > > > > specific slub_debug=F,ftrace_event_field) also hits this bug below. I
> > > > > > wanted to bisect it, but v5.7 was also bad, and also v5.6. Didn't try
> > > > > > further in history. So it's not new at all, and likely very specific to
> > > > > > your config+QEMU? (and related to the ACPI error messages that precede
> > > > > > it?).
> > [...]
> > [    0.140408] ------------[ cut here ]------------
> > [    0.140837] cache_from_obj: Wrong slab cache. Acpi-Namespace but object is from kmalloc-64
> > [    0.141406] WARNING: CPU: 0 PID: 1 at mm/slab.h:524 kmem_cache_free+0x1d3/0x250

Ah yes! Good. I had improved this check recently too, and I was worried
the freelist pointer patch was somehow blocking it, but I see now that
the failing config didn't have CONFIG_SLAB_FREELIST_HARDENED=y. Once
SLAB_CONSISTENCY_CHECKS was enabled ("slub_debug=F"), it started
tripping. Whew.

I wonder if that entire test block should just be removed from
cache_from_obj():

        if (!memcg_kmem_enabled() &&
            !IS_ENABLED(CONFIG_SLAB_FREELIST_HARDENED) &&
            !unlikely(s->flags & SLAB_CONSISTENCY_CHECKS))
                return s;

and make this test unconditional? It's mostly only called during free(),
and shouldn't be too expensive to be made unconditional. Hmm.

> > And it seems ACPI is allocating an object via kmalloc() and then freeing it
> > via kmem_cache_free(<"Acpi-Namespace" kmem_cache>) which is wrong.
> > 
> > > ./scripts/faddr2line vmlinux 'acpi_ns_root_initialize+0xb6'
> > acpi_ns_root_initialize+0xb6/0x2d1:
> > kmalloc at include/linux/slab.h:555
> > (inlined by) kzalloc at include/linux/slab.h:669
> > (inlined by) acpi_os_allocate_zeroed at include/acpi/platform/aclinuxex.h:57
> > (inlined by) acpi_ns_root_initialize at drivers/acpi/acpica/nsaccess.c:102
> > 
> 
> That's it :-) This fixes it for me:
> 
> diff --git a/drivers/acpi/acpica/nsaccess.c b/drivers/acpi/acpica/nsaccess.c
> index 2566e2d4c7803..b76bbab917941 100644
> --- a/drivers/acpi/acpica/nsaccess.c
> +++ b/drivers/acpi/acpica/nsaccess.c
> @@ -98,14 +98,12 @@ acpi_status acpi_ns_root_initialize(void)
>                  * predefined names are at the root level. It is much easier
> to
>                  * just create and link the new node(s) here.
>                  */
> -               new_node =
> -                   ACPI_ALLOCATE_ZEROED(sizeof(struct
> acpi_namespace_node));
> +               new_node = acpi_ns_create_node(*ACPI_CAST_PTR (u32,
> init_val->name));
>                 if (!new_node) {
>                         status = AE_NO_MEMORY;
>                         goto unlock_and_exit;
>                 }
> 
> -               ACPI_COPY_NAMESEG(new_node->name.ascii, init_val->name);
>                 new_node->descriptor_type = ACPI_DESC_TYPE_NAMED;
>                 new_node->type = init_val->type;

I'm a bit confused by the internals of acpi_ns_create_note(). It can still
end up calling ACPI_ALLOCATE_ZEROED() via acpi_os_acquire_object(). Is
this fix correct?

-- 
Kees Cook