From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1l2eMk-00074E-Bc for mharc-grub-devel@gnu.org; Thu, 21 Jan 2021 13:07:30 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:40752) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2eMi-00072o-0y for grub-devel@gnu.org; Thu, 21 Jan 2021 13:07:28 -0500 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:38614) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1l2eMg-0004NT-7N for grub-devel@gnu.org; Thu, 21 Jan 2021 13:07:27 -0500 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 10LI2i9O163749; Thu, 21 Jan 2021 13:07:22 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : subject : from : reply-to : to : cc : date : in-reply-to : references : content-type : mime-version : content-transfer-encoding; s=pp1; bh=Gm7vf75rjI6hmyW85cWQLLe0EwlNfxesYaLFsqirE6k=; b=KXTpVvGv1AlWE2cYVkALYJkAW5xEInyvmSvdByzZip1snXTudEdbjX8LDRlQzGS/eIwc OoLMy60OraNemn3g34TMqftABUOpymHq0mYnbwf7MYh1YzcM+NXjkZJ9zHaELu2Zxjb7 UcZT2gIsBIEuLzwJosjmJ46un9axv2dMHQql3t0dDRg6UqwhUlI2ubjRNXJ4RFs5VPFZ LGvpgeWoU5fIhOmSLuonQpRlVpzfcZ6tkfzy+/Me3kS9SGB/434WrCVKrz2+0L2Pd7GJ HTxnpL+LSVXCAlw0/vpBUwYaiYO9E7FZ/3PRPFSyo0ScqjPYPN5aW8/NEWArrsfJjWst Tg== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com with ESMTP id 367e1b98s4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Jan 2021 13:07:20 -0500 Received: from m0098417.ppops.net (m0098417.ppops.net [127.0.0.1]) by pps.reinject (8.16.0.36/8.16.0.36) with SMTP id 10LI2vPj164846; Thu, 21 Jan 2021 13:07:16 -0500 Received: from ppma01dal.us.ibm.com (83.d6.3fa9.ip4.static.sl-reverse.com [169.63.214.131]) by mx0a-001b2d01.pphosted.com with ESMTP id 367e1b98fs-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Jan 2021 13:07:15 -0500 Received: from pps.filterd (ppma01dal.us.ibm.com [127.0.0.1]) by ppma01dal.us.ibm.com (8.16.0.42/8.16.0.42) with SMTP id 10LI6X5Q001390; Thu, 21 Jan 2021 18:07:02 GMT Received: from b03cxnp07027.gho.boulder.ibm.com (b03cxnp07027.gho.boulder.ibm.com [9.17.130.14]) by ppma01dal.us.ibm.com with ESMTP id 3668nw9dy6-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 21 Jan 2021 18:07:02 +0000 Received: from b03ledav004.gho.boulder.ibm.com (b03ledav004.gho.boulder.ibm.com [9.17.130.235]) by b03cxnp07027.gho.boulder.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 10LI6wow8585538 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Thu, 21 Jan 2021 18:06:58 GMT Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id C4D297805E; Thu, 21 Jan 2021 18:06:58 +0000 (GMT) Received: from b03ledav004.gho.boulder.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 4C1FA7805C; Thu, 21 Jan 2021 18:06:56 +0000 (GMT) Received: from jarvis.int.hansenpartnership.com (unknown [9.85.161.248]) by b03ledav004.gho.boulder.ibm.com (Postfix) with ESMTP; Thu, 21 Jan 2021 18:06:56 +0000 (GMT) Message-ID: <03b43b6af0580028f185265eae3795cbe28ddd99.camel@linux.ibm.com> Subject: Re: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key From: James Bottomley Reply-To: jejb@linux.ibm.com To: development@efficientek.com 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 Date: Thu, 21 Jan 2021 10:06:55 -0800 In-Reply-To: <20210105140328.2ff45249@crass-HP-ZBook-15-G2> 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> <20210105140328.2ff45249@crass-HP-ZBook-15-G2> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.34.4 MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.343, 18.0.737 definitions=2021-01-21_09:2021-01-21, 2021-01-21 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 malwarescore=0 clxscore=1015 lowpriorityscore=0 suspectscore=0 adultscore=0 mlxscore=0 bulkscore=0 phishscore=0 mlxlogscore=999 impostorscore=0 priorityscore=1501 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000 definitions=main-2101210091 Received-SPF: pass client-ip=148.163.158.5; envelope-from=jejb@linux.ibm.com; helo=mx0b-001b2d01.pphosted.com X-Spam_score_int: -26 X-Spam_score: -2.7 X-Spam_bar: -- X-Spam_report: (-2.7 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_EF=-0.1, RCVD_IN_DNSWL_LOW=-0.7, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, 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: Thu, 21 Jan 2021 18:07:28 -0000 On Tue, 2021-01-05 at 14:03 -0600, Glenn Washburn wrote: > 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. Well, I've seen the patch, but it doesn't seem to address the root of the problem of where the password data might be used: if the password data is used by more than one function then it should likely be part of the shared data; if it's only used by a single function it makes more sense as an argument. I think you need to flesh your RFC out further to make that determination. On what is passed, we do have a question about whether it's data or a functional callback. I do tend to prefer callbacks in situations like this because they solve the lifetime issues (secrets should have well defined lifetimes to make sure there's a limited window for leaking them). A simple data pointer doesn't necessarily do this. So I think the important question is functional callback or data and where it's passed is simply a more minor detail the solution to which will become apparent in the use cases and which it's not hugely important to get right in the first instance. James > > > 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.