linux-kernel - Re: [PATCH v3] x86: Treat R_386_PLT32 as R_386

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]
Message-ID: <20210125172956.j2prlchhqwfcgzuc@google.com>
Date:   Mon, 25 Jan 2021 09:29:56 -0800
From:   Fangrui Song <maskray@...gle.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, clang-built-linux@...glegroups.com,
        Arnd Bergmann <arnd@...db.de>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Michael Matz <matz@...e.de>
Subject: Re: [PATCH v3] x86: Treat R_386_PLT32 as R_386_PC32


On 2021-01-25, Borislav Petkov wrote:
>It's a good thing I have a toolchain guy who can explain to me what you
>guys are doing because you need to start writing those commit messages
>for !toolchain developers.

How about this following message? I'll answer your questions in line as
well. Explaining everything in the message will be quite long...  If
someone is interested, I have put every possibly related matter in
https://maskray.me/blog/2021-01-09-copy-relocations-canonical-plt-entries-and-protected


This is similar to commit b21ebf2fb4cd ("x86: Treat R_X86_64_PLT32 as
R_X86_64_PC32"), but for i386.  As far as Linux kernel is concerned,
R_386_PLT32 can be treated the same as R_386_PC32.

R_386_PLT32/R_X86_64_PLT32 are PC-relative relocation types which can
only be used by branches. If the referenced symbol is defined
externally, a PLT will be used.
R_386_PC32/R_X86_64_PC32 are PC-relative relocation types which can be
used by address taking operations and branches. If the referenced symbol
is defined externally, a copy relocation/canonical PLT entry will be
created in the executable.

On x86-64, there is no PIC vs non-PIC PLT distinction and an
R_X86_64_PLT32 relocation is produced for both `call/jmp foo` and
`call/jmp foo@...` with newer (2018) GNU as/LLVM integrated assembler.
This avoids copy relocations/canonical PLT entries.

On i386, there are 2 types of PLTs, PIC and non-PIC. Currently the
GCC/GNU as convention is to use R_386_PC32 for non-PIC PLT and
R_386_PLT32 for PIC PLT. Copy relocations/canonical PLT entries are
possible ABI issues but GCC/GNU as will likely keep the status quo
because (1) the ABI is legacy (2) the change will drop a GNU ld
diagnostic for non-default visibility ifunc in shared objects.
https://sourceware.org/bugzilla/show_bug.cgi?id=27169

clang-12 -fno-pic (since
https://github.com/llvm/llvm-project/commit/a084c0388e2a59b9556f2de0083333232da3f1d6)
can emit R_386_PLT32 for compiler generated function declarations,
because preventing canonical PLT entries is weighed over the rare ifunc
diagnostic.

Link: https://github.com/ClangBuiltLinux/linux/issues/1210
Reported-by: Arnd Bergmann <arnd@...db.de>
Signed-off-by: Fangrui Song <maskray@...gle.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
Reviewed-by: Nathan Chancellor <natechancellor@...il.com>
Tested-by: Nick Desaulniers <ndesaulniers@...gle.com>
Tested-by: Nathan Chancellor <natechancellor@...il.com>


>On Thu, Jan 14, 2021 at 02:48:19PM -0800, Fangrui Song wrote:
>> This is similar to commit b21ebf2fb4cd ("x86: Treat R_X86_64_PLT32 as
>> R_X86_64_PC32"), but for i386.  As far as Linux kernel is concerned,
>> R_386_PLT32 can be treated the same as R_386_PC32.
>>
>> R_386_PC32/R_X86_64_PC32 are PC-relative relocation types with the
>> requirement that the symbol address is significant.
>> R_386_PLT32/R_X86_64_PLT32 are PC-relative relocation types without the
>> address significance requirement.
>
>I was told what "significant" means in that context and while it is
>clear to you, I'm pretty sure it is not clear to kernel developers who
>haven't looked at toolchains in depth. So please elaborate.

Expanded "significant" to more words. See above.

>> On x86-64, there is no PIC vs non-PIC PLT distinction and an
>> R_X86_64_PLT32 relocation is produced for both `call/jmp foo` and
>> `call/jmp foo@...` with newer (2018) GNU as/LLVM integrated assembler.
>
>Also, please explain in short why LLVM is generating R_X86_64_PLT32
>relocs now? I.e., is it the same reason as why binutils does that?
>
>I.e., mentioning the big picture of things would help as to why you're
>doing this.

It has been explained. The LLVM change was in 2018, roughly the same
time when GNU as emitted R_X86_64_PLT32. I think it does not need
extended explanation because of the separate canonical PLT entries
paragraph.

>> On i386, there are 2 types of PLTs, PIC and non-PIC. Currently the
>> convention is to use R_386_PC32 for non-PIC PLT and R_386_PLT32 for PIC
>> PLT.
>
>Convention in general or convention for LLVM?

Changed to "GCC/GNU as convention".

>> clang-12 -fno-pic (since
>> https://github.com/llvm/llvm-project/commit/a084c0388e2a59b9556f2de0083333232da3f1d6)
>> can emit R_386_PLT32 for compiler generated function declarations as
>> well to avoid a canonical PLT entry (st_shndx=0, st_value!=0) if the
>> symbol turns out to be defined externally. GCC/GNU as will likely keep
>> using R_386_PC32 because (1) the ABI is legacy (2) the change will drop
>> a GNU ld non-default visibility ifunc for shared objects.
>> https://sourceware.org/bugzilla/show_bug.cgi?id=27169
>
>Not sure how useful this paragraph is for kernel developers...

Reorganize it a bit...

>> Link: https://github.com/ClangBuiltLinux/linux/issues/1210
>> Reported-by: Arnd Bergmann <arnd@...db.de>
>> Signed-off-by: Fangrui Song <maskray@...gle.com>
>> Reviewed-by: Nick Desaulniers <ndesaulniers@...gle.com>
>> Reviewed-by: Nathan Chancellor <natechancellor@...il.com>
>> Tested-by: Nick Desaulniers <ndesaulniers@...gle.com>
>> Tested-by: Nathan Chancellor <natechancellor@...il.com>
>>
>> ---
>> Change in v2:
>> * Improve commit message
>> ---
>> Change in v3:
>> * Change the GCC link to the more relevant GNU as link.
>> * Fix the relevant llvm-project commit id.
>> ---
>>  arch/x86/kernel/module.c | 1 +
>>  arch/x86/tools/relocs.c  | 2 ++
>>  2 files changed, 3 insertions(+)
>>
>> diff --git a/arch/x86/kernel/module.c b/arch/x86/kernel/module.c
>> index 34b153cbd4ac..5e9a34b5bd74 100644
>> --- a/arch/x86/kernel/module.c
>> +++ b/arch/x86/kernel/module.c
>> @@ -114,6 +114,7 @@ int apply_relocate(Elf32_Shdr *sechdrs,
>>  			*location += sym->st_value;
>>  			break;
>>  		case R_386_PC32:
>> +		case R_386_PLT32:
>>  			/* Add the value, subtract its position */
>>  			*location += sym->st_value - (uint32_t)location;
>>  			break;
>> diff --git a/arch/x86/tools/relocs.c b/arch/x86/tools/relocs.c
>> index ce7188cbdae5..717e48ca28b6 100644
>> --- a/arch/x86/tools/relocs.c
>> +++ b/arch/x86/tools/relocs.c
>> @@ -867,6 +867,7 @@ static int do_reloc32(struct section *sec, Elf_Rel *rel, Elf_Sym *sym,
>>  	case R_386_PC32:
>>  	case R_386_PC16:
>>  	case R_386_PC8:
>> +	case R_386_PLT32:
>>  		/*
>>  		 * NONE can be ignored and PC relative relocations don't
>>  		 * need to be adjusted.
>
>That comment might need adjustment.
>
>> @@ -910,6 +911,7 @@ static int do_reloc_real(struct section *sec, Elf_Rel *rel, Elf_Sym *sym,
>>  	case R_386_PC32:
>>  	case R_386_PC16:
>>  	case R_386_PC8:
>> +	case R_386_PLT32:
>>  		/*
>>  		 * NONE can be ignored and PC relative relocations don't
>>  		 * need to be adjusted.
>
>Ditto.
>
>-- 
>Regards/Gruss,
>    Boris.
>
>https://people.kernel.org/tglx/notes-about-netiquette