From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kw1lZ-0000DW-Ap for mharc-grub-devel@gnu.org; Sun, 03 Jan 2021 06:41:45 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:44314) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kw0uc-0002z6-D7 for grub-devel@gnu.org; Sun, 03 Jan 2021 05:47:02 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:26490 helo=mx0a-001b2d01.pphosted.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kw0ua-0002W2-6r for grub-devel@gnu.org; Sun, 03 Jan 2021 05:47:02 -0500 Received: from pps.filterd (m0098414.ppops.net [127.0.0.1]) by mx0b-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 103A3DSW006989; Sun, 3 Jan 2021 05:46:55 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=subject : to : cc : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=ZSjtT6nm0ICwPzvTC046U0K0lm9e4ekSmGSDbJS3JGI=; b=sGBLbxjN2VsoFQhyEzB4CdFx31zvvA4/GZivHvEsbRYIXG8+7hz5iAjPv4NmB3wJP+e8 eXgUWoiSobqnVJaFiNz7gsFH2nK6syC32fmNgPXhI0iXgYYTQRN+ock1MHMlVgbjoO9n dBOgFUfaW8cmYxjAqGUJE2RaJ2cXpp1D9MiLna9Lxi34fEeOFOQdECJgPTlvRFN4Jv8N 3AZcg2Oy7wZnyQN329hRHeEc2EPYOuvadPJhJvL0lyD6hincNU7V3apcw1aCB0CGxSi3 IKyjHVdwzVG60w7s0wTloJQ6MNhQ3KB1Ou44eg1AHcguINfFd1gAP96iBhw5blRLLs68 0w== Received: from pps.reinject (localhost [127.0.0.1]) by mx0b-001b2d01.pphosted.com with ESMTP id 35ubc7gs81-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 03 Jan 2021 05:46:55 -0500 Received: from m0098414.ppops.net (m0098414.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 103AYsS6093508; Sun, 3 Jan 2021 05:46:54 -0500 Received: from ppma05fra.de.ibm.com (6c.4a.5195.ip4.static.sl-reverse.com [149.81.74.108]) by mx0b-001b2d01.pphosted.com with ESMTP id 35ubc7gs7v-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 03 Jan 2021 05:46:54 -0500 Received: from pps.filterd (ppma05fra.de.ibm.com [127.0.0.1]) by ppma05fra.de.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 103Agjkl027979; Sun, 3 Jan 2021 10:46:53 GMT Received: from b06avi18626390.portsmouth.uk.ibm.com (b06avi18626390.portsmouth.uk.ibm.com [9.149.26.192]) by ppma05fra.de.ibm.com with ESMTP id 35tgf88faq-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 03 Jan 2021 10:46:52 +0000 Received: from b06wcsmtp001.portsmouth.uk.ibm.com (b06wcsmtp001.portsmouth.uk.ibm.com [9.149.105.160]) by b06avi18626390.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 103AklDQ30540282 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sun, 3 Jan 2021 10:46:47 GMT Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E81ECA4054; Sun, 3 Jan 2021 10:46:49 +0000 (GMT) Received: from b06wcsmtp001.portsmouth.uk.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 7BCCCA405B; Sun, 3 Jan 2021 10:46:46 +0000 (GMT) Received: from [9.160.113.181] (unknown [9.160.113.181]) by b06wcsmtp001.portsmouth.uk.ibm.com (Postfix) with ESMTP; Sun, 3 Jan 2021 10:46:46 +0000 (GMT) Subject: Re: [PATCH v3 3/3] efi: Add API for retrieving the EFI secret for cryptodisk To: James Bottomley , grub-devel@gnu.org Cc: Dov.Murik1@il.ibm.com, ashish.kalra@amd.com, brijesh.singh@amd.com, tobin@ibm.com, david.kaplan@amd.com, jon.grimm@amd.com, thomas.lendacky@amd.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" References: <20201231173618.20751-1-jejb@linux.ibm.com> <20201231173618.20751-4-jejb@linux.ibm.com> From: Dov Murik Message-ID: Date: Sun, 3 Jan 2021 12:46:44 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 MIME-Version: 1.0 In-Reply-To: <20201231173618.20751-4-jejb@linux.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.343, 18.0.737 definitions=2021-01-03_02:2020-12-31, 2021-01-02 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 malwarescore=0 suspectscore=0 bulkscore=0 adultscore=0 mlxlogscore=999 clxscore=1011 phishscore=0 impostorscore=0 mlxscore=0 lowpriorityscore=0 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101030062 Received-SPF: none client-ip=148.163.158.5; envelope-from=dovmurik@linux.vnet.ibm.com; helo=mx0a-001b2d01.pphosted.com X-Spam_score_int: -37 X-Spam_score: -3.8 X-Spam_bar: --- X-Spam_report: (-3.8 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_EF=-0.1, NICE_REPLY_A=-1.118, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H2=-0.001, SPF_HELO_NONE=0.001, SPF_NONE=0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-Mailman-Approved-At: Sun, 03 Jan 2021 06:41:43 -0500 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 03 Jan 2021 10:47:02 -0000 Hello James, On 31/12/2020 19:36, James Bottomley wrote: > This module is designed to provide an efisecret command which > interrogates the EFI configuration table to find the location of the > confidential computing secret and tries to register the secret with > the cryptodisk. > > The secret is stored in a boot allocated area, usually a page in size. > The layout of the secret injection area is a header > > |GRUB_EFI_SECRET_TABLE_HEADER_GUID|len| > > with entries of the form > > |guid|len|data| > > the guid corresponding to the disk encryption passphrase is > GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string. > To get a high entropy string that doesn't need large numbers of > iterations, use a base64 encoding of 33 bytes of random data. > > Signed-off-by: James Bottomley > > --- > > v2: use callback to print failure message and destroy secret > v3: change to generic naming to use for TDX and SEV and use new mechanism > --- > grub-core/Makefile.core.def | 8 ++ > grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++ > include/grub/efi/api.h | 15 ++++ > 3 files changed, 152 insertions(+) > create mode 100644 grub-core/disk/efi/efisecret.c > > diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def > index 68b9e9f68..0b7e365cd 100644 > --- a/grub-core/Makefile.core.def > +++ b/grub-core/Makefile.core.def > @@ -785,6 +785,14 @@ module = { > enable = efi; > }; > > +module = { > + name = efisecret; > + > + common = disk/efi/efisecret.c; > + > + enable = efi; > +}; > + > module = { > name = lsefimmap; > > diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c > new file mode 100644 > index 000000000..318af0784 > --- /dev/null > +++ b/grub-core/disk/efi/efisecret.c > @@ -0,0 +1,129 @@ > +#include > +#include > +#include > +#include > +#include > +#include > + > +GRUB_MOD_LICENSE ("GPLv3+"); > + > +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID; > +static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SECRET_TABLE_HEADER_GUID; > +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID; > + > +/* > + * EFI places the secret in the lower 4GB, so it uses a UINT32 > + * for the pointer which we have to transform to the correct type > + */ This comment is no longer accurate now that efi_secret has 64-bit address and length fields. Just remove it? > +struct efi_secret { > + grub_uint64_t base; > + grub_uint64_t size; > +}; > + > +struct secret_header { > + grub_efi_packed_guid_t guid; > + grub_uint32_t len; > +}; > + > +struct secret_entry { > + grub_efi_packed_guid_t guid; > + grub_uint32_t len; > + char data[0]; > +}; > + > +static grub_err_t > +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it, > + char **ptr) > +{ > + struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct secret_entry *)0)->data); > + > + /* destroy the secret */ > + grub_memset (e, 0, e->len); In grub-core/lib/libgcrypt/src/g10lib.h there's a wipememory macro which should circumvent optimizers which might remove the grub_memset call: /* To avoid that a compiler optimizes certain memset calls away, these macros may be used instead. */ #define wipememory2(_ptr,_set,_len) do { \ volatile char *_vptr=(volatile char *)(_ptr); \ size_t _vlen=(_len); \ while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \ } while(0) #define wipememory(_ptr,_len) wipememory2(_ptr,0,_len) Maybe the same thing should be used here or copied here? This is an attempt to wipe the disk decryption password (= part the EFI secret area) against future leakage. (Note that I have no evidence that any optimizer currently removes the grub_memset call.) > + *ptr = NULL; > + > + if (have_it) > + return GRUB_ERR_NONE; > + > + return grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock any volumes"); > +} > + > +static grub_err_t > +grub_efi_secret_find (struct efi_secret *s, char **secret_ptr) > +{ > + int len; > + struct secret_header *h; > + struct secret_entry *e; > + unsigned char *ptr = (unsigned char *)(unsigned long)s->base; The cast to unsigned long can be removed (s->base is a grub_uint64_t). > + > + /* the area must be big enough for a guid and a u32 length */ > + if (s->size < sizeof (*h)) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small"); > + > + h = (struct secret_header *)ptr; > + if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid))) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not start with correct guid\n"); > + if (h->len < sizeof (*h)) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small\n"); > + > + len = h->len - sizeof (*h); > + ptr += sizeof (*h); > + > + while (len >= (int)sizeof (*e)) { > + e = (struct secret_entry *)ptr; > + if (e->len < sizeof(*e) || e->len > (unsigned int)len) > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is corrupt\n"); > + > + if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e->guid))) { > + int end = e->len - sizeof(*e); > + > + /* > + * the passphrase must be a zero terminated string because the > + * password routines call grub_strlen () to find its size > + */ > + if (e->data[end - 1] != '\0') > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is not zero terminated\n"); I think there's a pathological edge case if e->len exactly equals sizeof(struct secret_entry) -- that is, it indicates a GUID struct with a zero-length data field. In such a case, 'end' will be 0, the zero-termination check below might succeed because e->data[-1] == '\0' (last byte of length field in little-endian machine will probably be zero). As a result secret_ptr will point to garbage. A possible solution is to modify the above condition to: if (end <= 0 || e->data[end - 1] != '\0') return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is not zero terminated\n"); I assume that a 0-length data field might be OK for other GUIDs but it is not valid for the disk password GUID. > + > + *secret_ptr = e->data; > + return GRUB_ERR_NONE; > + } > + ptr += e->len; > + len -= e->len; > + } > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret aread does not contain disk decryption password\n"); typo: s/aread/area/ > +} > + > +static grub_err_t > +grub_efi_secret_get (const char *arg __attribute__((unused)), char **ptr) > +{ > + unsigned int i; > + > + for (i = 0; i < grub_efi_system_table->num_table_entries; i++) > + { > + grub_efi_packed_guid_t *guid = > + &grub_efi_system_table->configuration_table[i].vendor_guid; > + > + if (! grub_memcmp (guid, &secret_guid, sizeof (grub_efi_packed_guid_t))) { > + struct efi_secret *s = > + grub_efi_system_table->configuration_table[i].vendor_table; > + > + return grub_efi_secret_find(s, ptr); > + } > + } > + return grub_error (GRUB_ERR_BAD_ARGUMENT, "No secret found in the EFI configuration table"); > +} > + > +static struct grub_secret_entry secret = { > + .name = "efisecret", > + .get = grub_efi_secret_get, > + .put = grub_efi_secret_put, > +}; > + > +GRUB_MOD_INIT(efisecret) > +{ > + grub_cryptodisk_add_secret_provider (&secret); > +} > + > +GRUB_MOD_FINI(efisecret) > +{ > + grub_cryptodisk_remove_secret_provider (&secret); > +} > diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h > index 39733585b..53a47f658 100644 > --- a/include/grub/efi/api.h > +++ b/include/grub/efi/api.h > @@ -299,6 +299,21 @@ > { 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \ > } > > +#define GRUB_EFI_SECRET_TABLE_GUID \ > + { 0xadf956ad, 0xe98c, 0x484c, \ > + { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \ > + } > + > +#define GRUB_EFI_SECRET_TABLE_HEADER_GUID \ > + { 0x1e74f542, 0x71dd, 0x4d66, \ > + { 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b } \ > + } > + > +#define GRUB_EFI_DISKPASSWD_GUID \ > + { 0x736869e5, 0x84f0, 0x4973, \ > + { 0x92, 0xec, 0x06, 0x87, 0x9c, 0xe3, 0xda, 0x0b } \ > + } > + > #define GRUB_EFI_ACPI_TABLE_GUID \ > { 0xeb9d2d30, 0x2d88, 0x11d3, \ > { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \ > -Dov