From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mXBMC-0002bp-UG for mharc-grub-devel@gnu.org; Sun, 03 Oct 2021 19:57:24 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:39850) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mXBMB-0002af-Cg for grub-devel@gnu.org; Sun, 03 Oct 2021 19:57:23 -0400 Received: from mail-qk1-x72f.google.com ([2607:f8b0:4864:20::72f]:46656) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mXBM8-0002dk-UR for grub-devel@gnu.org; Sun, 03 Oct 2021 19:57:23 -0400 Received: by mail-qk1-x72f.google.com with SMTP id b65so14833961qkc.13 for ; Sun, 03 Oct 2021 16:57:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references:reply-to :mime-version:content-transfer-encoding; bh=c7qE6k0zOEA4u+WFZGypYq8NRhITOB+xcetwOGMM8yo=; b=kGhLv87XdaFApK8652EDOd81sxcoWqxa8Q9C8HIXSAcQMKUXaDJgsvVxTfan1ZhFCq CyVxw5kQv3qUxrQKIesZBlgC4sndvnFJ7UAr45ZG8qftCoCjmtUbiMv33yukWWlen0Ga uAHJzL9l81gJbcGoSCbVDIYE7L1hMsiC/X7/Xc6RdbaDpgrDn/ggszXPu4ckFCJyTAfG ik5C3K6p+G6r+IqqWaFsxh3B2N/A/F3Q2/60hov/WHBEDQ80E/Lf3IyLw01Ihlw9HP39 zIiyviANYEbIutGd5wKLNv72ltTJiDzwG06Nwqok5RE3mE2pbUmIbg1CZNmPBrQ/OvzH VcVQ== 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=c7qE6k0zOEA4u+WFZGypYq8NRhITOB+xcetwOGMM8yo=; b=pKzQypNgE08o29dZjuk3ZKW8FTwYzqmKN+pMp9us3O6ieNeTUJnNBevp8tUVhpahVj /AqkB+nMLlhwSIeh/7Ow9ka0rlG4HMXOrt7ycZnWhS9qDtzkigB7zhfLuhcKywo5PDhE EgitctQsKReZiwV/bpKKHkPgigzLW+7MH/OOzqjJ1gFtBIfWLeHWft1hLq/PtZjfJ2S7 /xIxl4rh8Oa42aI//cq+dh+9eRaVgET3XHDhpbhGGNGSuI5uRj7D9Ey+Fsn07HZtZv5T YHCokX1hAxawWFHeqqqy9NcZen+ShzcR7NAD+63AnWY3igclnAVX+YCzGuqa7LWq5Ou6 sSqQ== X-Gm-Message-State: AOAM5312L1pNi6RaiUNh0fszxojvWGT/uINEOtXmepQOOEpv/fLtLF0k uNlYUf2K5YqLJ3ox3WviRKEFcZFAmYboZQ== X-Google-Smtp-Source: ABdhPJwQGc2VTQ4KbyXV49rlbhOmjvyVXHA/6RDkTq6x+rij5bRlGMqIXSgIcM0fni6NpIWDiYUF9Q== X-Received: by 2002:a37:9d54:: with SMTP id g81mr7584074qke.124.1633305438617; Sun, 03 Oct 2021 16:57:18 -0700 (PDT) Received: from crass-HP-ZBook-15-G2 ([37.218.244.251]) by smtp.gmail.com with ESMTPSA id v201sm6886676qkb.29.2021.10.03.16.57.17 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Oct 2021 16:57:18 -0700 (PDT) Date: Sun, 3 Oct 2021 18:57:11 -0500 From: Glenn Washburn To: Patrick Steinhardt Cc: grub-devel@gnu.org, Daniel Kiper , Denis 'GNUtoo' Carikli , James Bottomley Subject: Re: [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args Message-ID: <20211003185711.472f2158@crass-HP-ZBook-15-G2> In-Reply-To: References: <20210927231403.642857-1-development@efficientek.com> <20210927231403.642857-5-development@efficientek.com> Reply-To: development@efficientek.com X-Mailer: Claws Mail 3.18.0 (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::72f; envelope-from=development@efficientek.com; helo=mail-qk1-x72f.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 Oct 2021 23:57:24 -0000 On Sun, 3 Oct 2021 22:22:40 +0200 Patrick Steinhardt wrote: > On Mon, Sep 27, 2021 at 06:14:03PM -0500, Glenn Washburn wrote: > > The member found_uuid was never used by the crypto-backends, but was used to > > determine if a crypto-backend successfully mounted a cryptodisk with a given > > uuid. This is not needed however, because grub_device_iterate will return 1 > > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device > > will only return 1 if a search_uuid has been specified and a cryptodisk was > > successfully setup by a crypto-backend. So the return value of > > grub_cryptodisk_scan_device is equivalent to found_uuid. > > Is that always the case though? Most notably, `scan_device_real` will > only set `have_it = 1` in case we have recovered the key. If the > cryptodisk existed before and was found via `get_by_source_disk`, then > we wouldn't set `have_it` at all. This essentially means that we'd only > set `have_it` in case we found a new cryptodisk, but not if we return a > preexisting one. > > I don't know whether this behaviour is something we rely on. My gut > feeling says it's rather a bug in the current code, which seems to be > confirmed by the error message in the `if (!have_it)` case, which says > "no such cryptodisk found". We did find one, but it's not a new one. > > So in total I think your patch makes sense and fixes a bug, but the > description doesn't quite match reality. Good catch, I actually hadn't thought about this case, and inadvertently fixed this bug. I'll update the commit message to reflect this. I don't believe the behavior of this "bug" is relied on anywhere either. Glenn > > Patrick > > > Signed-off-by: Glenn Washburn > > --- > > grub-core/disk/cryptodisk.c | 9 ++++----- > > include/grub/cryptodisk.h | 1 - > > 2 files changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c > > index 5e153ee0a..033894257 100644 > > --- a/grub-core/disk/cryptodisk.c > > +++ b/grub-core/disk/cryptodisk.c > > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name, > > > > grub_cryptodisk_insert (dev, name, source); > > > > - cargs->found_uuid = 1; > > - > > goto cleanup; > > } > > goto cleanup; > > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name, > > > > if (err) > > grub_print_error (); > > - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0; > > + return (!err && cargs->search_uuid) ? 1 : 0; > > } > > > > static grub_err_t > > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args) > > > > if (state[0].set) /* uuid */ > > { > > + int found_uuid = 0; > > grub_cryptodisk_t dev; > > > > dev = grub_cryptodisk_get_by_uuid (args[0]); > > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args) > > > > cargs.check_boot = state[2].set; > > cargs.search_uuid = args[0]; > > - grub_device_iterate (&grub_cryptodisk_scan_device, &cargs); > > + found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs); > > > > - if (!cargs.found_uuid) > > + if (!found_uuid) > > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found"); > > return GRUB_ERR_NONE; > > } > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h > > index 230167ab0..f4afb9cbd 100644 > > --- a/include/grub/cryptodisk.h > > +++ b/include/grub/cryptodisk.h > > @@ -70,7 +70,6 @@ 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; > > -- > > 2.32.0 > >