linux-kernel - Re: [GIT PULL] tracing: Fixes to bootconfig memory management

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, 14 Sep 2021 11:01:31 -0700
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Steven Rostedt <rostedt@...dmis.org>,
        Mike Rapoport <rppt@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Linux-MM <linux-mm@...ck.org>
Subject: Re: [GIT PULL] tracing: Fixes to bootconfig memory management

On Tue, Sep 14, 2021 at 7:56 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> A couple of memory management fixes to the bootconfig code

These may be fixes, but they are too ugly to merit the tiny
theoretical leak fix.

All of these are just plain wrong:

> +static void *init_xbc_data_copy __initdata;
> +static phys_addr_t init_xbc_data_size __initdata;
> +               init_xbc_data_copy = copy;
> +               init_xbc_data_size = size + 1;
> +       memblock_free(__pa(init_xbc_data_copy), init_xbc_data_size);

because the xbc code already saves these as xbc_data/xbc_data_size and
that final free should just be done in xbc_destroy_all().

So this fix is pointlessly ugly to begin with.

But what I _really_ ended up reacting to was that

> +               memblock_free(__pa(copy), size + 1);

where that "copy" was allocated with

        copy = memblock_alloc(size + 1, SMP_CACHE_BYTES);

so it should damn well be free'd without any crazy "__pa()" games.

This is a memblock interface bug, plain and simple.

Mike - this craziness needs to just be fixed. If memblock_alloc()
returns a virtual address, then memblock_free() should take one.

And if somebody has physical addresses because they aren't freeing
previously allocated resources, but because they are initializing the
memblock data from physical resources, then it shouldn't be called
"memblock_free()".

Alternatively, it should just _all_ be done in physaddr_t - that would
at least be consistent. But it would be *bad*.

Let's just get these interfaces fixed. It might be as simple as having
a "memblock_free_phys()" interface, and doing a search-and-replace
with coccinelle of

     memblock_free(__pa(xyz), .. -> memblock_free(xyz, ...
     memblock_free(other, .. -> memblock_free_phys(other, ..

and adding the (trivial) internal helper functions to memblock,
instead of making the atcual _users_ of memblock do insanely stupid
and confusing things.

Doing that automatic replacement might need an intermediate to avoid
the ambiguous case - first translate

     memblock_free(__pa(xyz), .. -> memblock_free_sane(xyz, ..

and then do any remaining

     memblock_free(xyz, .. -> memblock_free_phys(xyz, ..

and then when there are no remaining cases of 'memblock_free()' left,
do a final rename

     memblock_free_sane(.. -> memblock_free(..

but the actual commit can and should be just a single commit that just
fixes 'memblock_free()' to have sane interfaces.

Happily at least the type ends up making sure that we don't have
subtle mistakes (ie physaddr_t is an integer type, and a virtual
pointer is a pointer, so any missed conversions would cause nice
compile-time errors).

I hadn't noticed this insanity until now, but now that I do, I really
don't want to add to the ugliness for some unimportant theoretical
leak fix.

The memblock code has had enough subtleties that having inconsistent
and illogical basic interfaces is certainly not a good idea.

               Linus