From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mNSzn-0008IA-K6 for mharc-grub-devel@gnu.org; Tue, 07 Sep 2021 00:46:07 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:37508) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mNSzi-00088l-TT for grub-devel@gnu.org; Tue, 07 Sep 2021 00:46:02 -0400 Received: from mail-il1-x131.google.com ([2607:f8b0:4864:20::131]:35516) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mNSzg-0006pj-JO for grub-devel@gnu.org; Tue, 07 Sep 2021 00:46:02 -0400 Received: by mail-il1-x131.google.com with SMTP id h29so8737563ila.2 for ; Mon, 06 Sep 2021 21:45:59 -0700 (PDT) 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=3L3klGpSXkYQYpVS1UMsx4c/aUDdjx6KM0WsXcWmD3w=; b=DmIknNDR0cG4GfAXSIrz3VcwT1AnnCDIFe6bd86euhORC4Kf5jzIQIp0dnn80PcGCQ Ck/VgpqMISYRMlUk6XFduCjbrrXRYbPnc/GcYhwcZYGRs7Y0/VQoaX+3qxUdzpskeHDj /luIuvYsIIoFR9Qp62VX+5ysvrBh9YPE9rP8cyW+ZMSsKXwQt8YGJ4Vwj9YjJWjws3XF 6WIeROVgXMfDQ8JWKBDpQ8TFLJUAY3/JY4gTLIrjv9jJIJN4Q4RAvkIcJY5Dd27WsDSu xHj986w0KlJSNdUU+TfytmwS2SiapoZWuKXiGTc1KT3HKNse9ioKL6MuolkA7LFvofww Xc/g== 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=3L3klGpSXkYQYpVS1UMsx4c/aUDdjx6KM0WsXcWmD3w=; b=V3iznu4NoSk7caiARpGMXM+CDGL82tjzRxGuB0I+aWQGFUG3Ugthoq6N4c4W6BjQ1a F1udwwzyWtzPHnYb3Q08340QflnRuix41XeQ9eLRm4Ygcc2wFvfY+RGpkH2D1kjba/ge +c/Ol1mbaxP7IVg3HzacuaPAIyuQwnP+lrCuOSO8x7acSVkwpvsdMegiEWWYpSV5GJj6 37xXWAW9lvjUgx8fxzWhZGFtpwPgPrEb8exeg/nwKzdpWEnpvxoacWSvtU5MuKuYLhh2 CtpuHQxx3j5EnHeeUfpS7yAUhiYzZDHLpfAuOBW7/Pxvd2Pnrq2abOZVE4KhF7PwB5CI SA/A== X-Gm-Message-State: AOAM532Vgoef3YV9L2ClY01aKkJGXwiwHkAWgpDVS2YKVxhCU3co0SVr 2H4xrr1QFb6rM6H/l4DPcNgGgg== X-Google-Smtp-Source: ABdhPJyyu5xOv0vdymcNtx7d24YSUvI43pMYgky1mxCvB7ZKX1i477KXpXxZcl2Wyzd3LfsJTbwEpA== X-Received: by 2002:a92:ddcc:: with SMTP id d12mr11053236ilr.187.1630989958950; Mon, 06 Sep 2021 21:45:58 -0700 (PDT) Received: from ubuntu (public-nat-04.vpngate.v4.open.ad.jp. [219.100.37.236]) by smtp.gmail.com with ESMTPSA id c11sm5988412ioh.50.2021.09.06.21.45.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 06 Sep 2021 21:45:58 -0700 (PDT) Date: Tue, 7 Sep 2021 04:43:18 +0000 From: Glenn Washburn To: Patrick Steinhardt Cc: grub-devel@gnu.org, Daniel Kiper Subject: Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Message-ID: <20210907044318.784a6f76@ubuntu> In-Reply-To: References: Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.17.8 (GTK+ 2.24.33; 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::131; envelope-from=development@efficientek.com; helo=mail-il1-x131.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: Tue, 07 Sep 2021 04:46:03 -0000 On Mon, 30 Aug 2021 19:55:59 +0200 Patrick Steinhardt wrote: > On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote: > > As an example, passing a password as a cryptomount argument is > > implemented. However, the backends are not implemented, so testing > > this will return a not implemented error. > > > > Signed-off-by: Glenn Washburn > > --- > > grub-core/disk/cryptodisk.c | 31 ++++++++++++++++++++++--------- > > grub-core/disk/geli.c | 4 ++++ > > grub-core/disk/luks.c | 4 ++++ > > grub-core/disk/luks2.c | 4 ++++ > > include/grub/cryptodisk.h | 8 ++++++++ > > 5 files changed, 42 insertions(+), 9 deletions(-) > > > > diff --git a/grub-core/disk/cryptodisk.c > > b/grub-core/disk/cryptodisk.c index 90f82b2d3..b966b19ab 100644 > > --- a/grub-core/disk/cryptodisk.c > > +++ b/grub-core/disk/cryptodisk.c > > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] = > > /* TRANSLATORS: It's still restricted to cryptodisks only. */ > > {"all", 'a', 0, N_("Mount all."), 0, 0}, > > {"boot", 'b', 0, N_("Mount all volumes with `boot' flag > > set."), 0, 0}, > > + {"password", 'p', 0, N_("Password to open volumes."), 0, > > ARG_TYPE_STRING}, {0, 0, 0, 0, 0, 0} > > }; > > > > @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev) > > } > > > > static grub_err_t > > -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t > > source) +grub_cryptodisk_scan_device_real (const char *name, > > + grub_disk_t source, > > + grub_cryptomount_args_t cargs) > > { > > grub_err_t err; > > grub_cryptodisk_t dev; > > @@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char > > *name, grub_disk_t source) if (!dev) > > continue; > > > > + *dev->cargs = *cargs; > > err = cr->recover_key (source, dev); > > + *dev->cargs = NULL; > > if (err) > > { > > cryptodisk_close (dev); > > Is there any specific reason why you choose to set `cargs` as a struct > field? Seeing uses like this makes me wonder whether it wouldn't be > better to pass in `cargs` as explicit arguments whenever required. > This would also automatically answer any lifetime questions which > arise. I'm not opposed to this. One of my original goals was to try and not change the function interfaces between cryptomount and the backends. I also was originally going to have the dev->cargs stick around for the lifetime of the dev, but I'm not remembering a good use case for that. I'll send another series with cargs being passed as an argument. > [snip] > > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > > index 13103ea6a..e2a4a3bf5 100644 > > --- a/grub-core/disk/luks.c > > +++ b/grub-core/disk/luks.c > > @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source, > > grub_size_t max_stripes = 1; > > char *tmp; > > > > + /* Keyfiles are not implemented yet */ > > + if (dev->cargs->key_data || dev->cargs->key_len) > > + return GRUB_ERR_NOT_IMPLEMENTED_YET; > > + > > err = grub_disk_read (source, 0, 0, sizeof (header), &header); > > if (err) > > return err; > > Hum. This makes me wonder whether we'll have to adjust all backends > whenever we add a new parameter to `cargs` to return > `NOT_IMPLEMENTED`. I fear that it won't scale nicely, and that it is > a recipe for passing unsupported arguments to backends even though > they don't know to handle them. > > Not sure about a nice alternative though. The only idea I have right > now is something like a capabilities bitfield exposed by backends: > only if a specific bit is set will we pass the corresponding field to > such a backend. This would allow us to easily centralize the logic > into a single function which only receives both the capabilities > bitset and the `cargs` struct. > > Other than that I really like where this is going. I see your concern, and it would be nice to have an elegant solution to it. The capability bitfield idea seems workable. I don't think this needs to be solved right now though. This is a problem with all proposed approaches. I think that this *could* lead to scalability issues, but that's likely way down the road (considering the rate at which we're adding args to cryptomount). Also, I don't think this patch series hampers any such solution. So I think we can punt on this for now. Does that sound reasonable? Glenn