From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1kwsYT-0008EE-HU for mharc-grub-devel@gnu.org; Tue, 05 Jan 2021 15:03:45 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:59906) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kwsYS-0008E8-MJ for grub-devel@gnu.org; Tue, 05 Jan 2021 15:03:44 -0500 Received: from mail-io1-xd29.google.com ([2607:f8b0:4864:20::d29]:33202) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kwsYQ-0005Fd-9i for grub-devel@gnu.org; Tue, 05 Jan 2021 15:03:44 -0500 Received: by mail-io1-xd29.google.com with SMTP id w18so565783iot.0 for ; Tue, 05 Jan 2021 12:03:41 -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=B5pqjH6hoWyzc0fBDxI/axm7h0/WhrV6fIzmQN9xuBk=; b=aOjbzRIaMHUKfcLGxXjMiFPjD2KnumPcC4p57Jre/30l9wpfw0oYLxSPQt5pqLpLJ6 5dHkyEftrKIViZxQWkhyC9eir4PYzJDtc1BYhjQrkIil6K6APoNoGGD1AhcRs/ySENyk oypN4XVi7ryKogR3+W2BrGW9WS7wtp5hGC6d2XRjdA3B/rYhYRQ+wWlsQWJLgM84AOnI z2LYRtL/6O2uQEMyH9TFgNY9jHkFndVkys9/opUx1ZbsOuQsBnuTGLR8yHlUaOa6JKl+ yzStsyhGDXRnx/lDCtfRCbRSTcAQupq/twO8XH97IeEk11+wmGK23KEu0KPVFLmtIUdq ZwAQ== 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=B5pqjH6hoWyzc0fBDxI/axm7h0/WhrV6fIzmQN9xuBk=; b=EW93DrNg7sFPnv57gvhWAea+ibpHO3QbmQR5NMuGi1y519L9TxajJ8gVX6JEplsXXe yrqtBsrI5OZ0OjSuVOzoBiU7yNJQYOd5kgiDvrsdtKeVdQQY23gnqsfvfyX8Ttj59O1N HyIBQGGri4OJrYFD6DmFeAqLmSc+8hWOxXE2PF4BrwhOQ4/XXqbKXNvq6TC6FAwSi0vX r9YN3vQ82NbSIzODSqrzR91Rstk/CfuFnnnWV3zBR8xryD/U3c8yXahV4SQXUMS1x8MK FWfiQ/h016aw+NkH7cmTHI57dBAN4lS9xctP/Ra5lvdc6ExY4jNcXuLUrOgymFk3H5Q8 Z3Cg== X-Gm-Message-State: AOAM530sIFAODW08PqtZhw4z0fWXhzY4xq3wOxrPYwGu122VW3G//5HR I/pZNXelzr5ruV+1J7l62c5DWw== X-Google-Smtp-Source: ABdhPJyGkDo3AbWV7hX52736HtI4uqKdWk9SKy7IsgHLT/uWmQBxkp17dVQKFkHHBhkrDDec1qdh4Q== X-Received: by 2002:a02:b011:: with SMTP id p17mr1119415jah.114.1609877020944; Tue, 05 Jan 2021 12:03:40 -0800 (PST) Received: from crass-HP-ZBook-15-G2 ([136.49.211.192]) by smtp.gmail.com with ESMTPSA id o7sm128893iov.1.2021.01.05.12.03.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 05 Jan 2021 12:03:40 -0800 (PST) Date: Tue, 5 Jan 2021 14:03:28 -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: <20210105140328.2ff45249@crass-HP-ZBook-15-G2> In-Reply-To: <2eb69a4bab0ce70879b7bcf4535f04f00e596226.camel@linux.ibm.com> References: <20201231173618.20751-1-jejb@linux.ibm.com> <20201231173618.20751-2-jejb@linux.ibm.com> <20210102194348.5b0fa62f@crass-HP-ZBook-15-G2> <2eb69a4bab0ce70879b7bcf4535f04f00e596226.camel@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::d29; envelope-from=development@efficientek.com; helo=mail-io1-xd29.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, 05 Jan 2021 20:03:44 -0000 On Mon, 04 Jan 2021 22:48:49 -0800 James Bottomley wrote: > On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote: > > 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. > > Keeping large amounts of shared state between caller and callee can be > a debugger's nightmare. In this case the only consumer of the > password callback is the recover function, so it seems appropriate it > should be an argument to that function. Please see my recently submitted RFC patch for a more concrete idea of exactly what I'm suggesting. I don't see the concern about shared state as being applicable in this case, even though your point is valid in other situations. > > So, I don't think this is the right approach. > > The thing this patch demonstrates is that altering the function > signatures is fairly easy, so it would be a simple patch to alter them > again if the password callback were decided to be an essential > component of the cryptodisk device ... but it should really driven the > need which isn't apparent yet. By easy, I take you to mean there aren't a lot of places needing to be modified because of the change in signature, and in that sense its easy, which is good. But its also unnecessary. I'm not sure if you've looked at the struct grub_cryptodisk yet, but its already pretty cluttered. So I can see why you might not want to add more. However, I think my solution (again see RFC patch) is the cleaner, more scalable solution. Glenn