Received-SPF: pass (google.com: domain of linux-kernel+bounces-27438-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <f336c78c-0971-4494-bbb4-f0602cfdfb6e@siemens.com>
Date: Tue, 16 Jan 2024 14:56:42 +0100
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] riscv/efistub: Ensure GP-relative addressing is not used
Content-Language: en-US
To: Ard Biesheuvel <ardb@kernel.org>
Cc: Palmer Dabbelt <palmer@rivosinc.com>, linux-efi@vger.kernel.org,
 linux-kernel@vger.kernel.org
References: <mhng-8a4a5f85-faf9-405c-95a0-78cd04ec6509@palmer-ri-x1c9>
 <56b52d4e-9dc0-400c-a141-7e70f5c72afa@siemens.com>
 <CAMj1kXGR1aQdvej+0drfim-ZP27ZhO9UR_i_PT6F+hsV0UvbJw@mail.gmail.com>
 <578aae7c-4069-4071-ba4b-cc86d3b516c1@siemens.com>
 <CAMj1kXEC4=j5zcSyo91jnUkYMoqBG5M2raMXm4DNdD5ZbS3U7g@mail.gmail.com>
 <5c077064-bdab-4796-9ed6-8f884669b73f@siemens.com>
 <CAMj1kXGTmmNX0E4DOcJvYB2FYopudmOnPnY33OoT2RcYF0EG0g@mail.gmail.com>
From: Jan Kiszka <jan.kiszka@siemens.com>
In-Reply-To: <CAMj1kXGTmmNX0E4DOcJvYB2FYopudmOnPnY33OoT2RcYF0EG0g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?MktOeFNkaDh0NmEyMWM1djhNTXR0V25JT1NycnNFK0Q1MXBIRnoxb21LTUgr?=
 =?utf-8?B?ZXJVeGhOZDFrcll2c0NlTm5QbEVNV3FlaXgvWTJOTzE4M3o3THRYMHNKZnhS?=
 =?utf-8?B?c3V1YzNJck5LeEZFWGtOemV5RWVKSXZvaWFkaytXNjBPVFJtbWRpbndpcmY1?=
 =?utf-8?B?U05wQ09NLzFvUUVaL0JKeFRLeHo1QWszbUJnUG90d3czdmFGNGN4cW10aVdT?=
 =?utf-8?B?TDBHaG5RNnArVVlnTW5Iak54ZTFXUHdnaGxSNnR3K1N5cEhSbXV0Z0tIQ0lj?=
 =?utf-8?B?bzlFSkNLQy8vRy9HVTk0MW9QS3EwSk9va3lBeUJlMWtub01Mc0Z1ZDBOYlRW?=
 =?utf-8?B?dlF1SlRnVnRRT2JKTXA5allWY2xnelhWWHFlN2FqMHB0WmtiVExCdUYvMklR?=
 =?utf-8?B?REQ4Z201V09MOWFGSXJidmVWc2FTSDhXbjFsWmJLWmtFVmIxWm1xUC9raGNh?=
 =?utf-8?B?b2NabE02bjQ4T2l5OWwwVXdFWEZxbi9lOURNY0NHOXhtOTV2VUFYSkJ0c0VD?=
 =?utf-8?B?MjRuQnFhNmVWTDNEc1V4YTdKZVpjZVBjYjJRTW5JVGhvUVVkRVBCT0MvQzl5?=
 =?utf-8?B?UktNYzA4NTgwL3dHaHdSODk2RGJWOHhxK1FaOFJBOEN0RzNCVDIxbjJNN1pW?=
 =?utf-8?B?VWs5Tm9oSUpLOS80VlRLbTI3aWVvNWhJSVpHZ3dPVTNJQVhOYm95bC9DVUxM?=
 =?utf-8?B?dHBHeFM1K0d6akc1TzloeU9tdGZya0ZMYjJGdU5HRHphZjlScVMxNEtrUFBp?=
 =?utf-8?B?TlZjeWdVeUZWYzJ5LzB2YVVXZklMYkRGTC9UMEdEODVEK1QvRm9zR1Vwa3du?=
 =?utf-8?B?K3BLanp3S1EvSHhmSS90aHZ2a0x5SGg4YnRxeThWUTZ4UDQ0NlBYM1FEODJZ?=
 =?utf-8?B?WElteFNPYVU5OVRJVUEzajFyT0Q2TzhmSHlnMmdNajhlNnkzbXZlUnRYcDdn?=
 =?utf-8?B?SlNTQmJUV1pPU1U4U3ZmeHJtRDlmSGdSclFQemNoUGFBSjRmblVaZnNNYnFs?=
 =?utf-8?B?V01kc1RPOU9SejhaMGc3N1RBdmRISjFuTXRpUzh3WWo4ck1zYms4bTJ2eXVG?=
 =?utf-8?B?N3FVK1NzYUpxZzMwRk5Da0d2MG1qWkZ6RHdZaXZFTUhhOVVvSHBpdHNKVWw2?=
 =?utf-8?B?a01QM0pJSElKVFMyNzY1MXZuSVliaHhwQmJOUmZjZjQ4VFROREN4MjV2cW92?=
 =?utf-8?B?c3NndStYcWQ5U2lySXpDUGR4ejFERGxaTFlRWFFkUjNtL0JkR0p0bTZMYWRI?=
 =?utf-8?B?UGh0WGs2OEV3ZlFxdmh6T1B6ZmdWQmhOSjJPNFdxQUt0K0cwcFd4S0xiUEF0?=
 =?utf-8?B?b0RGNm1CVjVZM0FZeHJXMnZGWjc5anlOK1UxcG5UTTBXb2lHTm9lMEZMZEw3?=
 =?utf-8?B?dzRQK3lKNTlyNHVGWVJzb254VXprL3FKWm9KUzM2eXdOUVRhcllsREgrTGlq?=
 =?utf-8?B?d0xGZlNjbE1Ed0IrMEp1ZDBKeVVaZ1lrL3FtZ2F3NC9TNmNzOWpoNzdqWVNm?=
 =?utf-8?B?UjB2ZmZlMUYrYnB2ZjZWalp3TWtNYk9aVmJCQUVhU1lkZ21OaFFSZTI0QlVv?=
 =?utf-8?B?RFVLU0xmMzFRakczajVmUkxHS3hwanZORk1SMEJpV3J6SEhabmxwR1BNc0Fq?=
 =?utf-8?B?OE9nUGhnQjlDWTIxT0lDVS85a1BsV1JHa2laUXJuT0thYm1zbTFIYnowVFFC?=
 =?utf-8?B?NGthbTRSQU5TOU5HVWtLL25yR1ZmUUFtRjh5bVQvVTRzZ0tJc0c1VUVaYWlz?=
 =?utf-8?B?N0JobHoxMzdEZGdsSlc1YzVqZ1l6OVVUU0UyWko5NytORCtnaXkxdEhmVjZC?=
 =?utf-8?B?eXNBRmljUGVYZnBKeElEZ3FRUVBva0RvbitEWHU2SFZqSGtia3FVQWlYSTdq?=
 =?utf-8?B?eHI4Tm1GbEFwZTlpU1R3SkQ0Z2diMjArb3Z6cUhJVk1odHg1a0FrMjdhcTdK?=
 =?utf-8?B?aXFqZDhNTVdrbVZGS05VZUM4VDBKU2tuelpTRkc1VjVvWkRBMzlScDY5MlFX?=
 =?utf-8?B?eGVHRDhjYnR5YmxnOUZQMEVWSkVkSmVHeHptVzNON3l6YkZENzBIZEtRL0s0?=
 =?utf-8?B?RW9uc1AwT1c0WFVlUTRzOGJBWHcyWUhXSUl2Ri9yaExvRE9HNjVlZWJEeHZR?=
 =?utf-8?B?RHNhWmxYQjU2MmJ6clh4Z3dja1FPUG9oVnBqYjljWXkxaWdWcTlVMStDZ3BD?=
 =?utf-8?B?ZXc9PQ==?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 40623a63-70da-4b07-d07e-08dc169afac9
X-MS-Exchange-CrossTenant-AuthSource: AS4PR10MB6181.EURPRD10.PROD.OUTLOOK.COM
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Jan 2024 13:56:47.0011
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 38ae3bcd-9579-4fd4-adda-b42e1495d55a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: i9qoLhtHPVRjed7WT7U0v4DNP4C5bsdzAy28uVs4ahhPy/NqEgOBIKlA9BVmxKCZz5qO9r8VlLPJin3Y7NW/cA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM0PR10MB3283

On 16.01.24 14:47, Ard Biesheuvel wrote:
> On Tue, 16 Jan 2024 at 14:44, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>
>> On 16.01.24 09:36, Ard Biesheuvel wrote:
>>> On Tue, 16 Jan 2024 at 06:21, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>
>>>> On 15.01.24 18:34, Ard Biesheuvel wrote:
>>>>> On Sat, 13 Jan 2024 at 11:35, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>
>>>>>> On 12.01.24 19:56, Palmer Dabbelt wrote:
>>>>>>> On Fri, 12 Jan 2024 10:51:16 PST (-0800), Ard Biesheuvel wrote:
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> On Fri, 12 Jan 2024 at 19:37, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>>>>>>>>>
>>>>>>>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>>
>>>>>>>>> The cflags for the RISC-V efistub were missing -mno-relax, thus were
>>>>>>>>> under the risk that the compiler could use GP-relative addressing. That
>>>>>>>>> happened for _edata with binutils-2.41 and kernel 6.1, causing the
>>>>>>>>> relocation to fail due to an invalid kernel_size in handle_kernel_image.
>>>>>>>>> It was not yet observed with newer versions, but that may just be luck.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> Something like this should go to stable as well, but we will need
>>>>>>>>> rebased patches.
>>>>>>>>>
>>>>>>>>>  drivers/firmware/efi/libstub/Makefile | 2 +-
>>>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/firmware/efi/libstub/Makefile
>>>>>>>>> b/drivers/firmware/efi/libstub/Makefile
>>>>>>>>> index 06964a3c130f..d561d7de46a9 100644
>>>>>>>>> --- a/drivers/firmware/efi/libstub/Makefile
>>>>>>>>> +++ b/drivers/firmware/efi/libstub/Makefile
>>>>>>>>> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM)          += -DEFI_HAVE_STRLEN
>>>>>>>>> -DEFI_HAVE_STRNLEN \
>>>>>>>>>                                    -DEFI_HAVE_MEMCHR
>>>>>>>>> -DEFI_HAVE_STRRCHR \
>>>>>>>>>                                    -DEFI_HAVE_STRCMP -fno-builtin
>>>>>>>>> -fpic \
>>>>>>>>>                                    $(call
>>>>>>>>> cc-option,-mno-single-pic-base)
>>>>>>>>> -cflags-$(CONFIG_RISCV)         += -fpic -DNO_ALTERNATIVE
>>>>>>>>> +cflags-$(CONFIG_RISCV)         += -fpic -DNO_ALTERNATIVE -mno-relax
>>>>>>>>
>>>>>>>> Can we detect the presence of these references (via the relocation
>>>>>>>> type)? We already do something similar for ordinary absolute
>>>>>>>> references too.
>>>>>>>
>>>>>>> If there's no `__global_pointer$` symbol then the linker won't make
>>>>>>> GP-relative relaxations (because it doesn't know where GP is).  We
>>>>>>> usually define that symbol in the linker script, but I'm not entierly
>>>>>>> sure how libstub gets its linker script...
>>>>>>>
>>>>>>
>>>>>> The stub seems to be linked together with the rest of the kernel, thus
>>>>>> the regular arch/riscv/kernel/vmlinux.lds.S is used.
>>>>>>
>>>>>
>>>>> Indeed - the EFI stub is part of the same executable as vmlinux, we
>>>>> just mangle the symbol names to ensure that only code that can be
>>>>> safely called from the EFI stub can be linked to it.
>>>>>
>>>>> If the effect of -mno-relax is to stop emitting R_RISCV_RELAX
>>>>> relocations, we should perhaps add those to the STUBCOPY_RELOC-y
>>>>> Makefile variable? (in the same file). BTW R_RISCV_HI20 doesn't seem
>>>>> like the right value there to begin with: the idea of that is to
>>>>> disallow ELF relocations that evaluate to expressions that can only be
>>>>> known at runtime (like absolute addresses for global pointer
>>>>> variables)
>>>>
>>>> How to do that best? Simply replace R_RISCV_HI20 with R_RISCV_RELAX?
>>>>
>>>
>>> We'll need to keep the HI20, in fact - I got confused between HI20 and
>>> PCREL_HI20, and the former is actually used for 32-bit absolute
>>> addresses in 32-bit code.
>>>
>>> This seems to do the trick: it disallows relaxation relocations and
>>> native word sizes absolute references. AFAICT, those are the only ones
>>> we should care about.
>>>
>>> STUBCOPY_RELOC-$(CONFIG_RISCV) := -E
>>> R_RISCV_HI20\|R_RISCV_$(BITS)\|R_RISCV_RELAX
>>
>> I would suggest to do that on top of this patch. Want me to write such a
>> patch, or will you? You can probably more fluently explain why
>> R_RISCV_32/64 is important, I would first have to understand what that
>> is exactly. :)
>>
> 
> Sure, I can take care of that.
> 

Perfect.

> For your patch,
> 
> Reviewed-by: Ard Biesheuvel <ardb@kernel.org>
> 
> I'll queue this up as a EFI fix.

Thanks. Will you take care of stable, or should I once the commit was
merged?

Jan

-- 
Siemens AG, Technology
Linux Expert Center