Received-SPF: pass (google.com: domain of linux-crypto+bounces-4693-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <d6ec13f6-85ad-4c2e-a91e-36b834e30ffd@oracle.com>
Date: Tue, 4 Jun 2024 10:22:29 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v9 19/19] x86: EFI stub DRTM launch support for Secure
 Launch
To: Ard Biesheuvel <ardb@kernel.org>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
        linux-integrity@vger.kernel.org, linux-doc@vger.kernel.org,
        linux-crypto@vger.kernel.org, kexec@lists.infradead.org,
        linux-efi@vger.kernel.org, iommu@lists.linux-foundation.org,
        dpsmith@apertussolutions.com, tglx@linutronix.de, mingo@redhat.com,
        bp@alien8.de, hpa@zytor.com, dave.hansen@linux.intel.com,
        mjg59@srcf.ucam.org, James.Bottomley@hansenpartnership.com,
        peterhuewe@gmx.de, jarkko@kernel.org, jgg@ziepe.ca,
        luto@amacapital.net, nivedita@alum.mit.edu,
        herbert@gondor.apana.org.au, davem@davemloft.net, corbet@lwn.net,
        ebiederm@xmission.com, dwmw2@infradead.org, baolu.lu@linux.intel.com,
        kanth.ghatraju@oracle.com, andrew.cooper3@citrix.com,
        trenchboot-devel@googlegroups.com, ross.philipson@oracle.com
References: <20240531010331.134441-1-ross.philipson@oracle.com>
 <20240531010331.134441-20-ross.philipson@oracle.com>
 <CAMj1kXGC7a5+5at7T7M_mxBNWjqnuM4QGydG4ZEbu63y6fri3g@mail.gmail.com>
Content-Language: en-US
From: ross.philipson@oracle.com
In-Reply-To: <CAMj1kXGC7a5+5at7T7M_mxBNWjqnuM4QGydG4ZEbu63y6fri3g@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
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?b08raTRlaThMV3hvc2RkVm5FR0grcE4rd05uSWF4WWNKbXhVODd4YjAwMmFk?=
 =?utf-8?B?QUhIaEdsZmNKNFg3dzZ5WllUYUJKTURsd2R1bldXL3Q4QUFUeXBCRlluMlRJ?=
 =?utf-8?B?UytlTlgxU0F3anJ0Z1RIL0dxbTRLdmlIcHE3Tm05SkJXclpscEE0WnMvNlp3?=
 =?utf-8?B?Y3ZuLy9SR3RCM3dEcXROYnQ2RlZhcFA0dmtxaUNRNCtTWWdETUI2YjRyS2Rm?=
 =?utf-8?B?R1Z0YXgveUZyRGV1b1AyR1kvOGtNMW9jWVFTRzBFdllZOVNWdjQ3U1JLUFFa?=
 =?utf-8?B?UUdNTWJNN1MwZUJoaXFrQWZsQkNrRDgxcnZYM1JkOFFqS2hXSzNGaUoyVktz?=
 =?utf-8?B?VkpaVXlSVlhSeThaUCtMcG9USmFEZXlDdldjaTJIbVFpQWFIMEo2Lys5elVV?=
 =?utf-8?B?OVowNTZpalpnbEF1dksrdEtvOUM5QTNTM3E5YXNCUXpvVW81YjVKMGhDVGd3?=
 =?utf-8?B?QVhBa2ljdnlRaW91TUxMZ3RoNmRUcEx6bGxGclA3UEduVHhoV0VpOVN5V3Bw?=
 =?utf-8?B?WXk2WnBTeGd2eTRWSG1tZGZoS0s2SkNvYXFabTJ3ZGtSS1o4dkd3VTBYcXUr?=
 =?utf-8?B?YmRESnJ2S0UxS1BTL3lvN2JxVFpvVTM2RVl5VUdOVHltRlNHYWsyRndTODl0?=
 =?utf-8?B?WkMvR1lLZ016K1JJTVU2cXhRSnhVakRMMHEvZzdheTFLTkJIdC9lR0R2eTBp?=
 =?utf-8?B?UDVlQzZ5enVlMjIwdG9QN05KbkQ1Kzl4WWVSOWl3QVFWcHdyNm95eUs5bjl1?=
 =?utf-8?B?TFBUVzdQeDFDUVZKZkkvRzhTajhuWDI3QXk0MHFXSDk5YUtWM2N5dzNOdndq?=
 =?utf-8?B?eTdNZUNSaVhIZnhZYTNGMG84VDZYVlV0c0RwK3FDN2JlTWZvZ0VJZ2ZoelZF?=
 =?utf-8?B?VTEzdkU5OVBjZ1d5Y2YxSUtadTl0Q3dNald4QUpSZDBTZGVaanRhTjZKWFl1?=
 =?utf-8?B?MTA0Q3ZpWi9pR1NjWDh1WHIzZ1ZOZW9LYkM2QXB1aGhLYzJ0WXFHdHprYVcr?=
 =?utf-8?B?QkRLL0dNTWFjTjVWT3JXTlFxbjcveWFkbGlyZlJMZmMxaW1YVjVxMWdPVU44?=
 =?utf-8?B?TzdDQjA1bGRVUnJadUZzK29Pajd1ZEtVelBHbzVtTm1nZVFjMjJmTUxiSlhN?=
 =?utf-8?B?VXJ1Q3ROZE11eVJrajlSQXBRSmV5RjFmVGVWdnVscVRGY3ZGOG42SWlaOXlv?=
 =?utf-8?B?ZVZNUHRFK2pZTHlGUHFaRHFLWkVpb3gvU3Z4eXg3TlZhTnV2bVpBbG9pWGs4?=
 =?utf-8?B?a0QzZUtTbkpaTWR3YTFKR05ZcHBSWlpFSkFkNDRrMHplYjdjMGQ4bFl0SEdD?=
 =?utf-8?B?WUxtU0tEaG5VVGdoYWFqczVZMExjM21FeDZmSk54QnJQWnpJRnpLU3E1aEtI?=
 =?utf-8?B?ZHR2Sm5rc1FhUi9Hc2RkeUhjTlVFc1ZCQXRWdXM1ZjY1NlMvQU1GWmVtWlRD?=
 =?utf-8?B?d0JSRUpHNFh1UENDSGJNTUk4SGt1KzBwME5UWFU3R1hNWkNpVnYrUWoxaHhl?=
 =?utf-8?B?VE5YTlZvWG9QcU5JaXEzUXEzYlVJd09SeXhKOGlqdzNRZWc0clQrL2N2amFJ?=
 =?utf-8?B?dEhmc1p3TGRZZElyUE5TT3l5c1dOQUlsbEdOZmlyYUdxc1MwNzF6aUhITVBR?=
 =?utf-8?B?ak9EbVNDczMrc0JHVTY2cWtQUVdrRjJWTUxla0lKSFIvRFBRTm5nSlJXQmJr?=
 =?utf-8?B?bkdMZVEvZXZNbTRBeWQrV0c2ajE3cmxkbVRuYUZOWGlMbmhmeTNPQ3pxakRw?=
 =?utf-8?B?TC9aSHY0MHNkYUdRMW5uUXpuYWQ0ZEpxK0p1bnJEdkpISG16OUZNM0pDY2FJ?=
 =?utf-8?B?KzBvL1J3QmhlL3FFOGtmNTQvQThrS1NyQ1BPYjNEMU1ReXFweFNZOFBVcVMv?=
 =?utf-8?B?OE9Gd3JaTlFkMGN3ZmlZRjh5dGRGVGhCNko2dlp6cDFoM2pvSDVtb29IS1dJ?=
 =?utf-8?B?YWFuMFJHNWU3RmNBWlYzUWFYZ0V1cnZDZTlpREZjcy9aeU9rd1IzSXhPNDMx?=
 =?utf-8?B?aFZiOExLQ2piTHFUZFl0aWcrQXpTeDdSUm5URmEvZzN3ckV2YUxaVHBCTDhn?=
 =?utf-8?B?L3pxQWxZVlJIaENJb3NpenBsN29vVHMrMHlrcDNKQzhUSUtZYnU3YXJzc2FF?=
 =?utf-8?B?K2QwelJkTytKTisxTHRraFNsRE1NRE5NUVRoOEc5ODA3elF0aU95SkR3TEor?=
 =?utf-8?B?bnc9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	RfI13UwGGfDQF/qN7Svys8l+dai6wYcYghRdnRyWP/5J3xCKfWkT9OKlEnf1e/EF8rloox4MgS8+i5pa7M1eR+nSst7VuFHeBrDC90dossvF7CJ4ZLgH+UfZ0K2wsMgx/9XIMfc50JygaRF6ujCGIQRhC88pmtqkixGn9TYeNvZYSFwQHez1U+ixMnmwxMR01eFTL4S7k8c50kpyjZ7mVpVj5ZpHm3EYAipJX3Raa4RxkSraRIbbur33/OToTvvZCGRGyCvgHnn5Z+HSd/snD5IU+kYOIUP6tXJyMs4ITvxILJlWhx/gHy+aZv74wMWLzjhv/M9S6CBSigbHmmYJZYoGB0kQdtzcvzz/Lh3hjscBX0XZx80E8gXL+B/sdqQU40RV7gYApt9nAhhW8UecrJ/8+A8Vctyx7dsaIMcObU8ah/a7WBqH46atDkRpIZTuiQNDzFqIvjelhi/uYDcsvft0F6BptHZ7fCGtusxcnUx4ucgc0DogzWKXgvGwEThVgwtGIf4D4z9xcMuITtjuDCrlOAWzuR+fT4+R1v7m0EFGtonX3Z+yGkEXY3p9yPMfRUpNxNmrfVIpt5kAFo1G+Js4cV1t+uPdFZSU5iE5lIQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 06e60858-4d2a-4eb1-43c4-08dc84baeb7c
X-MS-Exchange-CrossTenant-AuthSource: DS0PR10MB7224.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 Jun 2024 17:22:33.1399
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7dopZIj3bdSDxzWm6D+wnCzBRLlRBkbl3Xd1GAHfoFD552Tbj6yJQu95jz8jm+9SX0tCg+ot/w+c9dJk5VEk8Q6dCHxK7r0pQ+F8Wyg9C8s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH8PR10MB6291
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1039,Hydra:6.0.680,FMLib:17.12.28.16
 definitions=2024-06-04_09,2024-06-04_01,2024-05-17_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 phishscore=0 bulkscore=0
 adultscore=0 spamscore=0 mlxlogscore=999 suspectscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2405010000
 definitions=main-2406040139
X-Proofpoint-GUID: b-t_FbAEFC7sCUrQFG9xoBAaeuv4tfkp
X-Proofpoint-ORIG-GUID: b-t_FbAEFC7sCUrQFG9xoBAaeuv4tfkp

On 5/31/24 4:09 AM, Ard Biesheuvel wrote:
> On Fri, 31 May 2024 at 03:32, Ross Philipson <ross.philipson@oracle.com> wrote:
>>
>> This support allows the DRTM launch to be initiated after an EFI stub
>> launch of the Linux kernel is done. This is accomplished by providing
>> a handler to jump to when a Secure Launch is in progress. This has to be
>> called after the EFI stub does Exit Boot Services.
>>
>> Signed-off-by: Ross Philipson <ross.philipson@oracle.com>
> 
> Just some minor remarks below. The overall approach in this patch
> looks fine now.
> 
> 
>> ---
>>   drivers/firmware/efi/libstub/x86-stub.c | 98 +++++++++++++++++++++++++
>>   1 file changed, 98 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
>> index d5a8182cf2e1..a1143d006202 100644
>> --- a/drivers/firmware/efi/libstub/x86-stub.c
>> +++ b/drivers/firmware/efi/libstub/x86-stub.c
>> @@ -9,6 +9,8 @@
>>   #include <linux/efi.h>
>>   #include <linux/pci.h>
>>   #include <linux/stddef.h>
>> +#include <linux/slr_table.h>
>> +#include <linux/slaunch.h>
>>
>>   #include <asm/efi.h>
>>   #include <asm/e820/types.h>
>> @@ -830,6 +832,97 @@ static efi_status_t efi_decompress_kernel(unsigned long *kernel_entry)
>>          return efi_adjust_memory_range_protection(addr, kernel_text_size);
>>   }
>>
>> +#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
> 
> IS_ENABLED() is mostly used for C conditionals not CPP ones.
> 
> It would be nice if this #if could be dropped, and replaced with ... (see below)
> 
> 
>> +static bool efi_secure_launch_update_boot_params(struct slr_table *slrt,
>> +                                                struct boot_params *boot_params)
>> +{
>> +       struct slr_entry_intel_info *txt_info;
>> +       struct slr_entry_policy *policy;
>> +       struct txt_os_mle_data *os_mle;
>> +       bool updated = false;
>> +       int i;
>> +
>> +       txt_info = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_INTEL_INFO);
>> +       if (!txt_info)
>> +               return false;
>> +
>> +       os_mle = txt_os_mle_data_start((void *)txt_info->txt_heap);
>> +       if (!os_mle)
>> +               return false;
>> +
>> +       os_mle->boot_params_addr = (u32)(u64)boot_params;
>> +
> 
> Why is this safe?

The size of the boot_params_addr is a holdover from the legacy boot 
world when boot params were always loaded at a low address. We will 
increase the size of the field.

> 
>> +       policy = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_ENTRY_POLICY);
>> +       if (!policy)
>> +               return false;
>> +
>> +       for (i = 0; i < policy->nr_entries; i++) {
>> +               if (policy->policy_entries[i].entity_type == SLR_ET_BOOT_PARAMS) {
>> +                       policy->policy_entries[i].entity = (u64)boot_params;
>> +                       updated = true;
>> +                       break;
>> +               }
>> +       }
>> +
>> +       /*
>> +        * If this is a PE entry into EFI stub the mocked up boot params will
>> +        * be missing some of the setup header data needed for the second stage
>> +        * of the Secure Launch boot.
>> +        */
>> +       if (image) {
>> +               struct setup_header *hdr = (struct setup_header *)((u8 *)image->image_base + 0x1f1);
> 
> Could we use something other than a bare 0x1f1 constant here? struct
> boot_params has a struct setup_header at the correct offset, so with
> some casting of offsetof() use, we can make this look a lot more self
> explanatory.

Yes we can do this.

> 
> 
>> +               u64 cmdline_ptr, hi_val;
>> +
>> +               boot_params->hdr.setup_sects = hdr->setup_sects;
>> +               boot_params->hdr.syssize = hdr->syssize;
>> +               boot_params->hdr.version = hdr->version;
>> +               boot_params->hdr.loadflags = hdr->loadflags;
>> +               boot_params->hdr.kernel_alignment = hdr->kernel_alignment;
>> +               boot_params->hdr.min_alignment = hdr->min_alignment;
>> +               boot_params->hdr.xloadflags = hdr->xloadflags;
>> +               boot_params->hdr.init_size = hdr->init_size;
>> +               boot_params->hdr.kernel_info_offset = hdr->kernel_info_offset;
>> +               hi_val = boot_params->ext_cmd_line_ptr;
> 
> We have efi_set_u64_split() for this.

Ok I will use that then.

> 
>> +               cmdline_ptr = boot_params->hdr.cmd_line_ptr | hi_val << 32;
>> +               boot_params->hdr.cmdline_size = strlen((const char *)cmdline_ptr);;
>> +       }
>> +
>> +       return updated;
>> +}
>> +
>> +static void efi_secure_launch(struct boot_params *boot_params)
>> +{
>> +       struct slr_entry_dl_info *dlinfo;
>> +       efi_guid_t guid = SLR_TABLE_GUID;
>> +       dl_handler_func handler_callback;
>> +       struct slr_table *slrt;
>> +
> 
> ... a C conditional here, e.g.,
> 
> if (!IS_ENABLED(CONFIG_SECURE_LAUNCH))
>      return;
> 
> The difference is that all the code will get compile test coverage
> every time, instead of only in configs that enable
> CONFIG_SECURE_LAUNCH.
> 
> This significantly reduces the risk that your stuff will get broken
> inadvertently.

Understood, I will address these as you suggest.

> 
>> +       /*
>> +        * The presence of this table indicated a Secure Launch
>> +        * is being requested.
>> +        */
>> +       slrt = (struct slr_table *)get_efi_config_table(guid);
>> +       if (!slrt || slrt->magic != SLR_TABLE_MAGIC)
>> +               return;
>> +
>> +       /*
>> +        * Since the EFI stub library creates its own boot_params on entry, the
>> +        * SLRT and TXT heap have to be updated with this version.
>> +        */
>> +       if (!efi_secure_launch_update_boot_params(slrt, boot_params))
>> +               return;
>> +
>> +       /* Jump through DL stub to initiate Secure Launch */
>> +       dlinfo = slr_next_entry_by_tag(slrt, NULL, SLR_ENTRY_DL_INFO);
>> +
>> +       handler_callback = (dl_handler_func)dlinfo->dl_handler;
>> +
>> +       handler_callback(&dlinfo->bl_context);
>> +
>> +       unreachable();
>> +}
>> +#endif
>> +
>>   static void __noreturn enter_kernel(unsigned long kernel_addr,
>>                                      struct boot_params *boot_params)
>>   {
>> @@ -957,6 +1050,11 @@ void __noreturn efi_stub_entry(efi_handle_t handle,
>>                  goto fail;
>>          }
>>
>> +#if (IS_ENABLED(CONFIG_SECURE_LAUNCH))
> 
> ... and drop this #if as well.

Yes.

Thanks
Ross

> 
>> +       /* If a Secure Launch is in progress, this never returns */
>> +       efi_secure_launch(boot_params);
>> +#endif
>> +
>>          /*
>>           * Call the SEV init code while still running with the firmware's
>>           * GDT/IDT, so #VC exceptions will be handled by EFI.
>> --
>> 2.39.3
>>