linux-kernel - Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 23 Apr 2019 15:25:18 +0200
From:   Borislav Petkov <bp@...en8.de>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     Qian Cai <cai@....pw>, dave.hansen@...ux.intel.com,
        luto@...nel.org, peterz@...radead.org, tglx@...utronix.de,
        mingo@...hat.com, x86@...nel.org, linux-kernel@...r.kernel.org,
        Brijesh Singh <brijesh.singh@....com>
Subject: Re: [PATCH] x86/mm/mem_encrypt: fix a crash with kmemleak_scan

Hi Catalin,

sorry for the delay.

On Thu, Apr 18, 2019 at 10:50:15AM +0100, Catalin Marinas wrote:
> Kmemleak is basically a tri-colour marking tracing garbage collector [1]

Thanks for that - interesting read.

> but without automatic freeing. It relies on a graph of references
> (pointers) between various objects and the root of such graph is the
> .bss/.data.
> 
> free_init_pages() is called on memory regions outside .bss/.data like
> smp_locks, initrd, kernel init text which kmemleak does not track
> anyway. That said, kmemleak_free_part() is tolerant to unknown pointers,
> so we could call it directly from free_init_pages().

Ok, lemme think out loud for a bit here: kmemleak_scan() goes over
an object list and I guess in our particular case, the memory which
got freed in mem_encrypt_free_decrypted_mem() *was* in that list too,
leading to the crash.

Looking at the splat, it is in scan_gray_list() which would mean that
the object we freed was reachable from the root(s) in .bss.

Now, the docs say:

"The memory allocations via :c:func:`kmalloc`, :c:func:`vmalloc`,
:c:func:`kmem_cache_alloc` and
friends are traced and the pointers, together with additional
information like size and stack trace, are stored in a rbtree."

So I guess free_init_pages() should be somehow telling kmemleak, "hey,
just freed that object, pls adjust your tracking lists" no?

Because, otherwise, if we start sprinkling those kmemleak_free_part()
calls everywhere, that'll quickly turn into a game of whack-a-mole. And
we don't need that especially if kmemleak can easily be taught to handle
such cases.

IM *very* HO, of course. :-)

Here's where you tell me whether that even makes sense at all.

> There is Documentation/dev-tools/kmemleak.rst briefly mentioning the
> tracing garbage collector (although the Wikipedia link is no longer
> valid, it should be replaced with [1]). It doesn't, however, state why
> .bss/.data are special.

The fact that they're special is important info, I'd say.

Also,

> [1] https://en.wikipedia.org/wiki/Tracing_garbage_collection#Tri-color_marking

is nice. While reading, it made me think how our discussion would go if
we didn't have wikipedia. You'd probably say, go to the library and read
this and that section in this and that book on tri-color marking. :-)

Thx.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.