From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kvsTG-0000x6-VC for mharc-grub-devel@gnu.org; Sat, 02 Jan 2021 20:46:15 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:43456) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kvsTF-0000wx-90 for grub-devel@gnu.org; Sat, 02 Jan 2021 20:46:13 -0500 Received: from mail-pj1-x102f.google.com ([2607:f8b0:4864:20::102f]:50609) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kvsTC-0007fX-JL for grub-devel@gnu.org; Sat, 02 Jan 2021 20:46:12 -0500 Received: by mail-pj1-x102f.google.com with SMTP id lj6so7752400pjb.0 for ; Sat, 02 Jan 2021 17:46:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=GWo9KZSPN361nDWka1hVi3jsZ/QbYk3yb7sT0pMWhOA=; b=sh6egjGyN2ycH4ga6KEl6xQXFS/tKdI8rIvo41ERxO6Lo7i5aCNyK6WEXZVY3/H0UB dZ8QOj5jH6G0Qtx5gCigHiOOCSbVKzj1M7Of+QVXVw2BkFmlLYRfpd7L/pAdLjx9rMPO oWumCcPdw/FtSCu02h0mKUewi0ZwejtUF1v82ptCOOR9IGkQwxvqmnqG6IBGhzMB2kAP 7xJ76XEDpzMpSYaIyjTthwtm2HQln3VoCPHr7bkpKfWiIZPEbwcb5JUjqRl2ksizFBJM Hte6HddhD9OXffIOCJZJseyArAciPmKE8q5qvjaTU1xBOpKvFzFklYzwvkRl4o/oaYFd wRUw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=GWo9KZSPN361nDWka1hVi3jsZ/QbYk3yb7sT0pMWhOA=; b=pXwZNvCzWH3ohtHiy1q7D68DuWCqzS38O+4EGLDD60bxisKGBXcCjbtKRX7xVyR+KK +5Dt2WpjTbMa98vb2U0tjydndSlzO/FdBwFfpbdCXf/byurovTMbhqobrhVGbf2IL4R1 J2lpjVS9c4NvhLfOlWQBh3GCGtKKCY5+5O4OcVs71Y7OnZnn238S78ejeVQrocQD4mx/ CAmJxG6Ode3QHGhfrlEEhF+Rch8M3vSWItOyPbNAyR0Scpz3MTTIVGuE8hjQFH4RjIBi ZPVj44Q8B87i+IHBWxg81SvXZKFrNLLboCr2o80UsqrimZlGlb8FAofYPFJh9wHOb61K NQCw== X-Gm-Message-State: AOAM5312/eV+FXneaap8lsdNtxxAvW4Bn/R57hhPdqH4Q9V38BU1MxLf raVKN42WV5jo1C6hkr4Gb2bLTw== X-Google-Smtp-Source: ABdhPJyAy2ZGyLjM5HHgcLOcmOk7B8vkBiBI15iS6oeRozD4BklWDwsC1Se4jRthu2ttPjgpUXB1BQ== X-Received: by 2002:a17:90a:fb8e:: with SMTP id cp14mr23354324pjb.96.1609638365133; Sat, 02 Jan 2021 17:46:05 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([136.49.211.192]) by smtp.gmail.com with ESMTPSA id l141sm53247455pfd.124.2021.01.02.17.45.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 02 Jan 2021 17:46:04 -0800 (PST) Date: Sat, 2 Jan 2021 19:45:50 -0600 From: Glenn Washburn To: James Bottomley Cc: The development of GNU GRUB , thomas.lendacky@amd.com, ashish.kalra@amd.com, brijesh.singh@amd.com, david.kaplan@amd.com, jon.grimm@amd.com, tobin@ibm.com, frankeh@us.ibm.com, "Dr . David Alan Gilbert" , dovmurik@linux.vnet.ibm.com, Dov.Murik1@il.ibm.com Subject: Re: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key Message-ID: <20210102194348.5b0fa62f@crass-HP-ZBook-15-G2> In-Reply-To: <20201231173618.20751-2-jejb@linux.ibm.com> References: <20201231173618.20751-1-jejb@linux.ibm.com> <20201231173618.20751-2-jejb@linux.ibm.com> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.17.4 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Received-SPF: pass client-ip=2607:f8b0:4864:20::102f; envelope-from=development@efficientek.com; helo=mail-pj1-x102f.google.com X-Spam_score_int: -18 X-Spam_score: -1.9 X-Spam_bar: - X-Spam_report: (-1.9 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RCVD_IN_DNSWL_NONE=-0.0001, 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: Sun, 03 Jan 2021 01:46:13 -0000 James, I like the improvements here. However, I've been thinking more about this and other improvements that deal with passing parameters to modules used by cryptomount. I have some ideas that I've not had the time to fully investigate or code up proof of concepts. One idea is that we shouldn't be changing the function declaration of recover key, that is to say adding new parameters. Instead we should be adding the parameters to grub_cryptodisk_t and setting them in grub_cryptodisk_scan_device_real. This would satisfy needs of this patch series as well as the key file, detached header, sending password as cryptomount arg, and master key features without cluttering the function signature. So, I don't think this is the right approach. Glenn On Thu, 31 Dec 2020 09:36:16 -0800 James Bottomley wrote: > For AMD SEV environments, the grub boot password has to be retrieved > from a given memory location rather than prompted for. This means > that the standard password getter needs to be replaced with one that > gets the passphrase from the SEV area and uses that instead. Adding > the password getter as a passed in argument to recover_key() makes > this possible. > > Signed-off-by: James Bottomley > > --- > > v2: add conditional prompting to geli.c > v3: make getter specify prompt requirement > --- > grub-core/disk/cryptodisk.c | 2 +- > grub-core/disk/geli.c | 12 +++++++----- > grub-core/disk/luks.c | 12 +++++++----- > grub-core/disk/luks2.c | 12 +++++++----- > grub-core/lib/crypto.c | 4 ++++ > grub-core/osdep/unix/password.c | 4 ++++ > grub-core/osdep/windows/password.c | 4 ++++ > include/grub/cryptodisk.h | 6 +++++- > 8 files changed, 39 insertions(+), 17 deletions(-) > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > index b62835acc..c51c2edb8 100644 > --- a/grub-core/disk/cryptodisk.c > +++ b/grub-core/disk/cryptodisk.c > @@ -1011,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char > *name, grub_disk_t source) if (!dev) > continue; > > - err = cr->recover_key (source, dev); > + err = cr->recover_key (source, dev, grub_password_get); > if (err) > { > cryptodisk_close (dev); > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c > index 2f34a35e6..3d826104d 100644 > --- a/grub-core/disk/geli.c > +++ b/grub-core/disk/geli.c > @@ -398,7 +398,8 @@ configure_ciphers (grub_disk_t disk, const char > *check_uuid, } > > static grub_err_t > -recover_key (grub_disk_t source, grub_cryptodisk_t dev) > +recover_key (grub_disk_t source, grub_cryptodisk_t dev, > + grub_passwd_cb *password_get) > { > grub_size_t keysize; > grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN]; > @@ -438,11 +439,12 @@ recover_key (grub_disk_t source, > grub_cryptodisk_t dev) tmp = NULL; > if (source->partition) > tmp = grub_partition_get_name (source->partition); > - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), > source->name, > - source->partition ? "," : "", tmp ? : "", > - dev->uuid); > + if (password_get (NULL, 0)) > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), > source->name, > + source->partition ? "," : "", tmp ? : "", > + dev->uuid); > grub_free (tmp); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > + if (!password_get (passphrase, MAX_PASSPHRASE)) > return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not > supplied"); > /* Calculate the PBKDF2 of the user supplied passphrase. */ > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 13103ea6a..13eee2a18 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char > *check_uuid, > static grub_err_t > luks_recover_key (grub_disk_t source, > - grub_cryptodisk_t dev) > + grub_cryptodisk_t dev, > + grub_passwd_cb *password_get) > { > struct grub_luks_phdr header; > grub_size_t keysize; > @@ -187,11 +188,12 @@ luks_recover_key (grub_disk_t source, > tmp = NULL; > if (source->partition) > tmp = grub_partition_get_name (source->partition); > - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), > source->name, > - source->partition ? "," : "", tmp ? : "", > - dev->uuid); > + if (password_get (NULL, 0)) > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), > source->name, > + source->partition ? "," : "", tmp ? : "", > + dev->uuid); > grub_free (tmp); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > + if (!password_get (passphrase, MAX_PASSPHRASE)) > { > grub_free (split_key); > return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not > supplied"); diff --git a/grub-core/disk/luks2.c > b/grub-core/disk/luks2.c index 7460d7b58..7597d8576 100644 > --- a/grub-core/disk/luks2.c > +++ b/grub-core/disk/luks2.c > @@ -542,7 +542,8 @@ luks2_decrypt_key (grub_uint8_t *out_key, > > static grub_err_t > luks2_recover_key (grub_disk_t source, > - grub_cryptodisk_t crypt) > + grub_cryptodisk_t crypt, > + grub_passwd_cb *password_get) > { > grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN]; > char passphrase[MAX_PASSPHRASE], cipher[32]; > @@ -584,10 +585,11 @@ luks2_recover_key (grub_disk_t source, > /* Get the passphrase from the user. */ > if (source->partition) > part = grub_partition_get_name (source->partition); > - grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), > source->name, > - source->partition ? "," : "", part ? : "", > - crypt->uuid); > - if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > + if (password_get (NULL, 0)) > + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), > source->name, > + source->partition ? "," : "", part ? : "", > + crypt->uuid); > + if (!password_get (passphrase, MAX_PASSPHRASE)) > { > ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not > supplied"); goto err; > diff --git a/grub-core/lib/crypto.c b/grub-core/lib/crypto.c > index ca334d5a4..34272a7ad 100644 > --- a/grub-core/lib/crypto.c > +++ b/grub-core/lib/crypto.c > @@ -456,6 +456,10 @@ grub_password_get (char buf[], unsigned buf_size) > unsigned cur_len = 0; > int key; > > + if (!buf) > + /* want prompt */ > + return 1; > + > while (1) > { > key = grub_getkey (); > diff --git a/grub-core/osdep/unix/password.c > b/grub-core/osdep/unix/password.c index 9996b244b..365ac4bad 100644 > --- a/grub-core/osdep/unix/password.c > +++ b/grub-core/osdep/unix/password.c > @@ -34,6 +34,10 @@ grub_password_get (char buf[], unsigned buf_size) > int tty_changed = 0; > char *ptr; > > + if (!buf) > + /* want prompt */ > + return 1; > + > grub_refresh (); > > /* Disable echoing. Based on glibc. */ > diff --git a/grub-core/osdep/windows/password.c > b/grub-core/osdep/windows/password.c index 1d3af0c2c..2a6615611 100644 > --- a/grub-core/osdep/windows/password.c > +++ b/grub-core/osdep/windows/password.c > @@ -33,6 +33,10 @@ grub_password_get (char buf[], unsigned buf_size) > DWORD mode = 0; > char *ptr; > > + if (!buf) > + /* want prompt */ > + return 1; > + > grub_refresh (); > > GetConsoleMode (hStdin, &mode); > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > index dcf17fbb3..737487bb4 100644 > --- a/include/grub/cryptodisk.h > +++ b/include/grub/cryptodisk.h > @@ -112,6 +112,9 @@ struct grub_cryptodisk > }; > typedef struct grub_cryptodisk *grub_cryptodisk_t; > > +/* must match prototype for grub_password_get */ > +typedef int (grub_passwd_cb)(char buf[], unsigned buf_size); > + > struct grub_cryptodisk_dev > { > struct grub_cryptodisk_dev *next; > @@ -119,7 +122,8 @@ struct grub_cryptodisk_dev > > grub_cryptodisk_t (*scan) (grub_disk_t disk, const char > *check_uuid, int boot_only); > - grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t > dev); > + grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, > + grub_passwd_cb *get_password); > }; > typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t; >