linux-kernel - Re: [PATCH v2 11/13] lkdtm: Fix lkdtm_EXEC

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:   Sat, 16 Oct 2021 08:41:03 +0200
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Kees Cook <keescook@...omium.org>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
        Helge Deller <deller@....de>, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-ia64@...r.kernel.org, linux-parisc@...r.kernel.org,
        linux-arch@...r.kernel.org, linux-mm@...ck.org
Subject: Re: [PATCH v2 11/13] lkdtm: Fix lkdtm_EXEC_RODATA()

Le 15/10/2021 à 23:32, Kees Cook a écrit :
> On Thu, Oct 14, 2021 at 07:50:00AM +0200, Christophe Leroy wrote:
>> Behind its location, lkdtm_EXEC_RODATA() executes
>> lkdtm_rodata_do_nothing() which is a real function,
>> not a copy of do_nothing().
>>
>> So executes it directly instead of using execute_location().
>>
>> This is necessary because following patch will fix execute_location()
>> to use a copy of the function descriptor of do_nothing() and
>> function descriptor of lkdtm_rodata_do_nothing() might be different.
>>
>> And fix displayed addresses by dereferencing the function descriptors.
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@...roup.eu>
> 
> I still don't understand this -- it doesn't look needed at all given the
> changes in patch 12. (i.e. everything is using
> dereference_function_descriptor() now)

dereference_function_descriptor() only deals with the function address, 
not the function TOC.

do_nothing() is a function. It has a function descriptor with a given 
address (address of .do_nothing) and a given TOC, say TOC1.

lkdtm_rodata_do_nothing() is another function. It has its own function 
descriptor with a given address (address of .lkdtm_rodata_do_nothing) 
and a given TOC, say TOC2.

If we use execute_location(), it will copy do_nothing() function 
descriptor and change the function address to the address of 
lkdtm_rodata_do_nothing(). So it will call lkdtm_rodata_do_nothing() 
with TOC1 instead of calling it with TOC2.

> 
> Can't this patch be dropped?

It is likely that the TOC will be the same for both functions, and 
anyway those functions are so simple that they don't use the TOC at all, 
so yes it would likely work without this patch but from my point of view 
it is incorrect to call one function with the TOC from the descriptor of 
another function.

If you thing we can take the risk, then I'm happy to drop the patch and 
replace it by

	execute_location(dereference_function_descriptor(lkdtm_rodata_do_nothing), CODE_AS_IS)

Christophe