From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mPt8v-0003BN-Gb for mharc-grub-devel@gnu.org; Mon, 13 Sep 2021 17:05:34 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:50504) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mPt8r-00039z-9q for grub-devel@gnu.org; Mon, 13 Sep 2021 17:05:29 -0400 Received: from mail-oi1-x22a.google.com ([2607:f8b0:4864:20::22a]:41675) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mPt8p-0002uf-32 for grub-devel@gnu.org; Mon, 13 Sep 2021 17:05:28 -0400 Received: by mail-oi1-x22a.google.com with SMTP id 6so15853823oiy.8 for ; Mon, 13 Sep 2021 14:05:23 -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=Pguxs0Tp+GqKIx6KOgfuOCoeK2aT+un50mgBjSqE8qE=; b=m2/WC0EYbP3NoUUiM3hunX9efM8fxJrNMFs0eAN60Dg+YrphcWAVvMcftLRI23gI0X jatyFeYAVICwqMIjHpNxOac6CFjvF9sAQ2/HcdvgU3zXxGy/nSg/QiIcnlJIJsXShS18 Xe2rtZkdjXFi9Jg6NTMqHaLZELpHjNfOK0y+gftp2K64mIezAm/+6MjCrrnYASAua9hX JyrHrgGxCQ3q0ErCLstOSwcuIoVwXy2K7tvomPSYC6x6A/teH6W7sSaVbvcmRkaHqetj uae5c/vyJaerezu0YZqE/uv7nTP855k+OF6ilsg5kp+L2YG/c+XQkg07+S7dRokH/GJ0 6meA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:reply-to:mime-version:content-transfer-encoding; bh=Pguxs0Tp+GqKIx6KOgfuOCoeK2aT+un50mgBjSqE8qE=; b=49wkj3orS2aDD+Y5IZ7oQEGw1b1RGHs3kjGKzAgXIje4MM9EZJv16tLjxUzPJFcueh 8ixmY6oRcXX530pjgUC0I1iXoufICbmYWTS9NGdxMS5PnA5zP2kMUZyDHoAF+13beNeH ajmCGMbUcI9p+O4HDqROOD/lh27+lf6eoT55TdyXQpi7OzN78nPrHrPFrNZikR+LJy/i BxSMIYY4w26msF0EUojQ8m1RwWFkzjaP5LKDskeWXJf6qLFOKLtZbDf+ICXsBTpDXtrR 8Dva9d9Az+STxv1jJhBUqEX5wZ/UpjId5SJOv5DRqbrIfuTGQXIqVi0OlRytTQ45QbOy b/jg== X-Gm-Message-State: AOAM532DNes2nPcu6qX4mMFy/v8Vq2A0nb3YiupIh8vnIKOQF8dd4NUS CbyjCGJaPI0KVVdVyGmk0yOwLg== X-Google-Smtp-Source: ABdhPJwBfPzLsGlbFCP/XPXbjArRUQmzGEehGfj5WUQp/IaEvswQCYtP1cWWMaGBPY5mv7ZwfPN09g== X-Received: by 2002:aca:4b49:: with SMTP id y70mr9118764oia.16.1631567122810; Mon, 13 Sep 2021 14:05:22 -0700 (PDT) Received: from ubuntu ([187.195.2.105]) by smtp.gmail.com with ESMTPSA id y7sm2092081oov.36.2021.09.13.14.05.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 13 Sep 2021 14:05:22 -0700 (PDT) Date: Mon, 13 Sep 2021 21:05:15 +0000 From: Glenn Washburn To: Patrick Steinhardt Cc: grub-devel@gnu.org, Daniel Kiper Subject: Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct Message-ID: <20210913210515.5753cf48@ubuntu> In-Reply-To: References: <20210907023430.2eaff8a1@ubuntu> 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::22a; envelope-from=development@efficientek.com; helo=mail-oi1-x22a.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: Mon, 13 Sep 2021 21:05:29 -0000 On Sun, 12 Sep 2021 13:17:29 +0200 Patrick Steinhardt wrote: > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote: > > On Mon, 30 Aug 2021 20:02:26 +0200 > > Patrick Steinhardt wrote: > > > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote: > > > > Signed-off-by: Glenn Washburn > > > > --- > > > > grub-core/disk/cryptodisk.c | 26 +++++++++----------------- > > > > include/grub/cryptodisk.h | 3 +++ > > > > 2 files changed, 12 insertions(+), 17 deletions(-) > > > > > > > > diff --git a/grub-core/disk/cryptodisk.c > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644 > > > > --- a/grub-core/disk/cryptodisk.c > > > > +++ b/grub-core/disk/cryptodisk.c > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk) > > > > > > > > #endif > > > > > > > > -static int check_boot, have_it; > > > > -static char *search_uuid; > > > > - > > > > static void > > > > cryptodisk_close (grub_cryptodisk_t dev) > > > > { > > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char > > > > *name, > > > > FOR_CRYPTODISK_DEVS (cr) > > > > { > > > > - dev = cr->scan (source, search_uuid, check_boot); > > > > + dev = cr->scan (source, cargs->search_uuid, cargs->check_boot); > > > > if (grub_errno) > > > > return grub_errno; > > > > if (!dev) > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char > > > > *name, > > > > grub_cryptodisk_insert (dev, name, source); > > > > > > > > - have_it = 1; > > > > + cargs->found_uuid = 1; > > > > > > > > goto cleanup; > > > > } > > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char > > > > *sourcedev, const char *cheat) > > > > FOR_CRYPTODISK_DEVS (cr) > > > > { > > > > - dev = cr->scan (source, search_uuid, check_boot); > > > > + dev = cr->scan (source, NULL, 0); > > > > if (grub_errno) > > > > return grub_errno; > > > > if (!dev) > > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name, > > > > > > > > if (err) > > > > grub_print_error (); > > > > - return have_it && search_uuid ? 1 : 0; > > > > + return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0; > > > > } > > > > > > > > static grub_err_t > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t > > > > ctxt, int argc, char **args) cargs.key_len = > > > > grub_strlen(state[3].arg); } > > > > > > > > - have_it = 0; > > > > if (state[0].set) /* uuid */ > > > > { > > > > grub_cryptodisk_t dev; > > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t > > > > ctxt, int argc, char **args) return GRUB_ERR_NONE; > > > > } > > > > > > > > - check_boot = state[2].set; > > > > - search_uuid = args[0]; > > > > + cargs.check_boot = state[2].set; > > > > + cargs.search_uuid = args[0]; > > > > grub_device_iterate (&grub_cryptodisk_scan_device, &cargs); > > > > - search_uuid = NULL; > > > > > > > > - if (!have_it) > > > > + if (!cargs.found_uuid) > > > > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such > > > > cryptodisk found"); return GRUB_ERR_NONE; > > > > } > > > > else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */ > > > > { > > > > - search_uuid = NULL; > > > > - check_boot = state[2].set; > > > > + cargs.check_boot = state[2].set; > > > > grub_device_iterate (&grub_cryptodisk_scan_device, &cargs); > > > > - search_uuid = NULL; > > > > return GRUB_ERR_NONE; > > > > } > > > > else > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t > > > > ctxt, int argc, char **args) char *disklast = NULL; > > > > grub_size_t len; > > > > > > > > - search_uuid = NULL; > > > > - check_boot = state[2].set; > > > > + cargs.check_boot = state[2].set; > > > > diskname = args[0]; > > > > len = grub_strlen (diskname); > > > > if (len && diskname[0] == '(' && diskname[len - 1] == ')') > > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > > > > index 1070140d9..11062f43a 100644 > > > > --- a/include/grub/cryptodisk.h > > > > +++ b/include/grub/cryptodisk.h > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t > > > > > > > > struct grub_cryptomount_args > > > > { > > > > + grub_uint32_t check_boot : 1; > > > > + grub_uint32_t found_uuid : 1; > > > > + char *search_uuid; > > > > grub_uint8_t *key_data; > > > > grub_size_t key_len; > > > > }; > > > > > > Aren't these parameters in a different scope than the key data? These > > > are only used for device discovery via `scan()`, while the other ones > > > are for decrypting the key. Do we want to split those up into two > > > different structs? > > > > This struct is meant to be used for any data passed to the crypto > > backend from cryptomount. All of those members are affected by > > cryptomount options. So this struct isn't about anything in particular, > > just a common set of data passed to the crypto backends via > > cryptomount. So I don't think two structs would improve anything here. > > Am I missing something? > > > > Glenn > > I'm mostly wondering about lifetimes of these parameters. They are used > in different phases of the cryptomount: some are used only at the time > of discovery, while others are used at decryption time, where it's not > immediately clear which parameters are used when without having a look > at the cryptodisk implementations. That's why I was thinking it might > make more sense to split them up by those phases such that this becomes > explicit. > > Patrick Okay, I think I see what you're saying. Essentially, as someone wanting to write a new crypto-backend, when I'm writing by scan function, how do I know what fields in the cargs struct are valid (have been assigned)? The answer would be to check if they are not NULL, and otherwise they're available. I don't really see an issue. Would your objection be alleviated by having cargs be redefined like so: struct grub_cryptomount_args { struct { grub_uint32_t check_boot : 1; char *search_uuid; } scan; struct { grub_uint8_t *key_data; grub_size_t key_len; } recover_key; struct { ... } common; }; Then in scan you know you can only use scan and common structs. I have a common because the detached header changes will be such that scan and recover_key need to be provided with detached header. I think this way is more than is needed at the present moment. If I'm still not getting it, can you be a little more concrete in what is problematic and how you think it could be changed to be better? Glenn