From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1l2gKv-00024V-W4 for mharc-grub-devel@gnu.org; Thu, 21 Jan 2021 15:13:46 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:42786) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2gKu-00022H-OL for grub-devel@gnu.org; Thu, 21 Jan 2021 15:13:44 -0500 Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:58062) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2gKs-0000jg-H9 for grub-devel@gnu.org; Thu, 21 Jan 2021 15:13:44 -0500 Received: from pps.filterd (m0098394.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 10LKCgN6136451; Thu, 21 Jan 2021 15:13:39 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : subject : from : reply-to : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding; s=pp1; bh=ZOCkShkl2Xt1+5awfXvU7ewBGX8Au5LQSpTm0fhBw10=; b=TBlIEb2lHonWqM7kxjE9GjPpq0GGMq5TMlpRxWYg8R3DpnXQ7Xw34a07pqjJ0z14lqjv WMa6lQfYakh4QorcXfguAXRDHSk3j+chiWYcNGNHa4MERvoLLnu8mLZXRShL/SjVTuHq Exit8Py26H/Tfpxdhx1coTFt15vvOCitUXcF3C9fXGgqPh4sajf8h49eG0ygqltz+pGn t/8Tm106CBzKJdQNJor7kpww9cFvV1GC+r/Jfu2qf4QdrErl48ubW4TFn4aO5vUel/bk Obz2uRZupikm9zwZ9mjNRWfngzQ8wPuQE8aeaWCGuAqSZ4ChCfB8jhY4tyoZu4s3tQpa qA== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 367getg0mb-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Jan 2021 15:13:39 -0500 Received: from m0098394.ppops.net (m0098394.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 10LKDcmW141040; Thu, 21 Jan 2021 15:13:38 -0500 Received: from ppma01dal.us.ibm.com (83.d6.3fa9.ip4.static.sl-reverse.com [169.63.214.131]) by mx0a-001b2d01.pphosted.com with ESMTP id 367getg0m4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Jan 2021 15:13:38 -0500 Received: from pps.filterd (ppma01dal.us.ibm.com [127.0.0.1]) by ppma01dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 10LKCXDF028829; Thu, 21 Jan 2021 20:13:37 GMT Received: from b03cxnp07028.gho.boulder.ibm.com (b03cxnp07028.gho.boulder.ibm.com [9.17.130.15]) by ppma01dal.us.ibm.com with ESMTP id 3668nwaa0x-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Jan 2021 20:13:37 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp07028.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 10LKDYRU19333468 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 21 Jan 2021 20:13:34 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 473837805E; Thu, 21 Jan 2021 20:13:34 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3CAC578067; Thu, 21 Jan 2021 20:13:32 +0000 (GMT) Received: from jarvis.int.hansenpartnership.com (unknown [9.85.161.248]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 21 Jan 2021 20:13:31 +0000 (GMT) Message-ID: <92c768b228ebca5959916c47200ec836cc612dbc.camel@linux.ibm.com> Subject: Re: [PATCH v3 3/3] efi: Add API for retrieving the EFI secret for cryptodisk From: James Bottomley Reply-To: jejb@linux.ibm.com To: Dov Murik , 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" Date: Thu, 21 Jan 2021 12:13:30 -0800 In-Reply-To: References: <20201231173618.20751-1-jejb@linux.ibm.com> <20201231173618.20751-4-jejb@linux.ibm.com> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.34.4 MIME-Version: 1.0 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-21_10:2021-01-21, 2021-01-21 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 bulkscore=0 clxscore=1015 malwarescore=0 mlxlogscore=999 spamscore=0 lowpriorityscore=0 suspectscore=0 adultscore=0 priorityscore=1501 impostorscore=0 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101210100 Received-SPF: pass client-ip=148.163.156.1; envelope-from=jejb@linux.ibm.com; helo=mx0a-001b2d01.pphosted.com X-Spam_score_int: -26 X-Spam_score: -2.7 X-Spam_bar: -- X-Spam_report: (-2.7 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action 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: Thu, 21 Jan 2021 20:13:44 -0000 On Sun, 2021-01-03 at 12:46 +0200, Dov Murik wrote: > Hello James, > > On 31/12/2020 19:36, James Bottomley wrote: [...] > > 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? Yes, an oversight, I'll update the comment. > > +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.) Actually, this gets into the whole systems annoyance about who should see the secret and how long should it live. The truth is this secret has to be used twice: once for grub to find the kernel and initrd and once for the initrd to mount root. The fact that there's no current mechanism in place for grub to pass the secret to the initrd is a bit of a systems failure. I was thinking the config table could do that, in which case this code should really be eliminated. Likely the best thing to do would be to have an OVMF exitbootservices notifier which wipes everything. > > + *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 style of casting is best practice for converting integers to pointers ... it's actually what the C manual recommends so it's best to keep it for the compiler to be aware this is exactly what is intended. Just as an illustration think what happens on a 32 bit machine: grub_uint64_t is too big and we need the unsigned long cast to truncate it. > > + > > + /* 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. Sure, although I think end < 2 just in case a zero length string causes other problems. > > + > > + *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/ fixed. James