Received-SPF: pass (google.com: domain of linux-kernel+bounces-27430-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <5c077064-bdab-4796-9ed6-8f884669b73f@siemens.com>
Date: Tue, 16 Jan 2024 14:44:35 +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>
From: Jan Kiszka <jan.kiszka@siemens.com>
In-Reply-To: <CAMj1kXEC4=j5zcSyo91jnUkYMoqBG5M2raMXm4DNdD5ZbS3U7g@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?UHJubUlQSTMzM3lla0NweG55RkFWNXNOaXBZMjNSVi9Cb2dROVlsZ2dsL0Zn?=
 =?utf-8?B?QWJST1hWVm9kcFdST3I4WjI1cUpzekdnbThkNS9PNFR5VXlIZW9ra1ZkSk9r?=
 =?utf-8?B?a0pheTU0Zm1pV3ZjOFNjQ3A1RmVQTTdwMkcySyswQmxSdlV2bStOUDRwaWgx?=
 =?utf-8?B?dmk1SXpGTTVxUmhnU3ptaFdielg4Z2VMTWpKM04zYmxnSFNIZTdpOG5TNDc3?=
 =?utf-8?B?QTEwU1NXdXcvUmhQMmxDVlZETWdsU3ZOL1NRbTBUMnI5OFc3OUdNckczRm9q?=
 =?utf-8?B?TERqY3QzWWk0SDFjU1pqMnY0OTFxZG1meWFSSG9XNlZYd1c1UzFJak5vNllT?=
 =?utf-8?B?MnJySjVZbzFkcEdldEFUQmhUOC9rSDhCazlPVjhnTDFUTG1iMTNKM1lkamkv?=
 =?utf-8?B?N1NQUUNiZU1vZEFobTJPQ2t1b0F6Wjcvc1RtT052Y0IrNjJuSjdUWWNSM2J2?=
 =?utf-8?B?MEpKMkdUYTU1bW9JaDhkTHNkcVJtaHUxZ2xrb1FncCswYzhVdElYWlRJdVdH?=
 =?utf-8?B?RU1mcXlRUDhvbyswMWc4REs1SW4zRyt0dTRSVzFzTUlFelVZUU5Qd0VCM3A2?=
 =?utf-8?B?L01LUkV1OVhvZ0NvVXJVc0k3bUNGOFJ0ZVA4RkI3WDBpSzN6Skt1YS9SZllH?=
 =?utf-8?B?MExXY2F2cnY4eEd4d1ZDdnN6d205L0wrblFpaTIybEN3d0JaL2tPVURQbk5O?=
 =?utf-8?B?RHZrRHg5Zmg0WUZLZXJEMU13QXQvUnl5REJUYWNZakJzRnM5aXJsTEhOZ2hm?=
 =?utf-8?B?SUc2RUFzRTFIZFJQVWxxRkdzd0VSbFhFMHlBRXJJbFNSbUYzdmNEcUd6V1dl?=
 =?utf-8?B?cEhMYnZMaWlQRXlINnZVa2tYb1F4OVYvc09VUzFRRHo0R0hzVUVFdjBmOUhQ?=
 =?utf-8?B?bjNuZlZwVVVPWW1aeFFmZFlxUGxaRWJZYWgvc1J2SVpIUVBFc1dTZDgydld0?=
 =?utf-8?B?N2F3aFJVb0VxbmdTR05LMlg1NnZHbm9GQ3JCU2NDeFBIM1pWOXp5SE10ZEFE?=
 =?utf-8?B?T3BFeDJ2NDdCY1BQcGFZRSs2eGJOS1U0azBUekxvb3U5SlVhMzB0TkhGQVVZ?=
 =?utf-8?B?MSt5K2JIQVpnRGluNVhoM1B6MUJoVVVBb3NtemcyUTdBTWFBNVhuT3NRRUJz?=
 =?utf-8?B?UjdFMk9YSWlXYjlRdENqaFUybE1VODhzMHkrNjdEK3BDay9WRkZhdUlLMFNB?=
 =?utf-8?B?eG9UdFNnUDBuTThJaURGMzJ1VjZGdEEwb3JPYjBFczI2V3VlcTRidVVCTURD?=
 =?utf-8?B?Rk5mMHZrSlhJZEVSSDFkQ3J5eGd2ajRLQ2plQnU3RHpEYnliMlNYbG81eHoy?=
 =?utf-8?B?MURGR2dkbDZZU1E5U3dubHVwcFlPZmRxQjdyY25CVWhlYU5iWXFqZGNWTjNO?=
 =?utf-8?B?OXF4SmR3OCs3cUMrL1dEc01Qb0ZYYklGbjREMlRZdC9IUFJzb2dzK1JzRGJh?=
 =?utf-8?B?Z3FsVEVEZ1RqaHRpaUpUeVpyR0tpN0FuZzA1RUdRSXJzVEJLYlRobVhVN1hk?=
 =?utf-8?B?T1JoQWYrL3F0RE5XY2tObUQyT2FKTXpadzR4VTBnTXQ3NDR3dEFnTVNSL1NC?=
 =?utf-8?B?c3FmRmJDMXVKS004ZmppYnd3NncxdnNuSHo5dGZLb1c1RGFoVXZiU3VRWUc0?=
 =?utf-8?B?MHFDNXdoWmJ4ZHJOUFEvbHRUMUdzMFUwM0d5UklrblhwNEZYRlByRXpaM2pa?=
 =?utf-8?B?S3A0KzlPRm9aMmRtQXJLcndJV3ZzNzhUZG1TUGRSc2R3TjU2S3cvWFdYYnZi?=
 =?utf-8?B?eWtnU0hMNXplOGFROWUzYTgrbnk5WmxqOWlNTUVRYVhMNG1jcDROd1AwMEVH?=
 =?utf-8?B?Q1pKWXRtc0FVRUZZbHlYRDR6UTdveGVFbWxCK3VmcG5XdVRKclVKWnE0MDV5?=
 =?utf-8?B?emZ2SXhXMklBVFFKZnU2Q1lyTk5IaWJBQmlGVk5hMDhrSGJZSkpLVHdIQmVz?=
 =?utf-8?B?b2VrL1hhbEpIVzQwc2FkclpaWkpiYkkxQk9qS0oyS2p4S2QxSksrQUF5a3ZR?=
 =?utf-8?B?TS9Wb281WGg3VGJrRGtDVnlMT3JVYnNvOGNmSU1kUU5vMDh2UWlySWZFeHA0?=
 =?utf-8?B?ckQyN0ZhRjIveHhNZ2lzc211TmN3Y2kzZWxnUXZqd1lTNDJuei9GaXhSMTBz?=
 =?utf-8?B?SGZhTXN0Z1JlL0lXa0RzWDRUM0lGQUs1Wk1HZGRkR0FMZWhBd2ovbjlpTUtX?=
 =?utf-8?B?TVE9PQ==?=
X-OriginatorOrg: siemens.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 54c69806-1a42-4640-e2f0-08dc169947be
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:44:37.0606
 (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: jKlUyCYf15uDr3ZQaxGCDxVDfYTPirl49uxs72P0zUCw2F87F2mA/BJtVccf3nSR9cRjdMXToofSKZPaQmrfAg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB4PR10MB7037

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. :)

Jan

-- 
Siemens AG, Technology
Linux Expert Center