From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id EF640C433E0 for ; Mon, 13 Jul 2020 20:32:45 +0000 (UTC) Received: from merlin.infradead.org (merlin.infradead.org [205.233.59.134]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B54C82077D for ; Mon, 13 Jul 2020 20:32:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="TICXeH0G"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=linux.microsoft.com header.i=@linux.microsoft.com header.b="HKGa5xri" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B54C82077D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.microsoft.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=merlin.20170209; h=Sender:Content-Type: Content-Transfer-Encoding:Cc:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date:Message-ID:From: References:To:Subject:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=S66YzYt8GvD4APcyHOhMkJpzMrwME1q5sAvcki/Df98=; b=TICXeH0G/LC9TlpK+tDhf98qP szNfH7Kx/QD3uejW/tGdCqIwlVw8EjorazDmg4NKv0A2lTaDXkMrd+5z0pfr8yBRyc+0aLDpNZRQ0 S6JjXE3DIAg3iDQZtiDV7/ZLBUpsTs/Gg3qhTZvKkHZecuTtLfOY2KY9sKgCUzXrvDgrZVGrO/jVJ GdbRsdN1QGVrlcumuoZLCdSpNmlS2zC3HNEJ68dWOh0BDubl/PLP0CqywByCZ+Z+Aq+q0vnc+1JdW 5IE/j+O+cWklnZjWyXr7zAr+So2/MOfMOmJnZXDKwlixxHpcTpgsBi06NXOVRORXIuJEMINBMS9S+ y/S2CPMVA==; Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jv56R-0002bR-Gz; Mon, 13 Jul 2020 20:31:07 +0000 Received: from linux.microsoft.com ([13.77.154.182]) by merlin.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jv56N-0002an-VH for linux-arm-kernel@lists.infradead.org; Mon, 13 Jul 2020 20:31:05 +0000 Received: from [10.0.0.249] (c-24-19-135-168.hsd1.wa.comcast.net [24.19.135.168]) by linux.microsoft.com (Postfix) with ESMTPSA id 3468820B4908; Mon, 13 Jul 2020 13:31:00 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3468820B4908 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1594672261; bh=/alYulr7TfHdtnQcnR5ksYNOj4rEKwjLZlm8tyeN/ig=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=HKGa5xriPaJtfUMOGKAdDEmNm530YOlnYSEACRiZ3qek6FvrL/gO2JzOlCjjVpaES wr6E6LV/9mSxj+YP7UlkZ8qn3zs9VPhh9ADbVFJKR8LajLESGVLVlMgtnguuVmFup0 Kmtw0sgUMtC7d96W9/rPiy8/xnK4YEXPkWFcs8aE= Subject: Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima. To: Thiago Jung Bauermann References: <20200618071045.471131-1-prsriva@linux.microsoft.com> <20200618071045.471131-2-prsriva@linux.microsoft.com> <87o8per3m0.fsf@morokweng.localdomain> From: Prakhar Srivastava Message-ID: <1385c8bb-cd25-8dc4-7224-8e27135f3356@linux.microsoft.com> Date: Mon, 13 Jul 2020 13:30:59 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <87o8per3m0.fsf@morokweng.localdomain> Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200713_163104_255251_DA1984F9 X-CRM114-Status: GOOD ( 50.93 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: kstewart@linuxfoundation.org, mark.rutland@arm.com, gregkh@linuxfoundation.org, benh@kernel.crashing.org, bhsharma@redhat.com, tao.li@vivo.com, zohar@linux.ibm.com, paulus@samba.org, vincenzo.frascino@arm.com, will@kernel.org, nramas@linux.microsoft.com, frowand.list@gmail.com, masahiroy@kernel.org, jmorris@namei.org, takahiro.akashi@linaro.org, linux-arm-kernel@lists.infradead.org, catalin.marinas@arm.com, serge@hallyn.com, devicetree@vger.kernel.org, pasha.tatashin@soleen.com, robh+dt@kernel.org, hsinyi@chromium.org, tusharsu@linux.microsoft.com, tglx@linutronix.de, allison@lohutok.net, christophe.leroy@c-s.fr, mbrugger@suse.com, balajib@linux.microsoft.com, dmitry.kasatkin@gmail.com, linux-kernel@vger.kernel.org, linux-security-module@vger.kernel.org, james.morse@arm.com, mpe@ellerman.id.au, linux-integrity@vger.kernel.org, linuxppc-dev@lists.ozlabs.org Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote: > > Prakhar Srivastava writes: > >> Powerpc has support to carry over the IMA measurement logs. Refatoring the >> non-architecture specific code out of arch/powerpc and into security/ima. >> >> The code adds support for reserving and freeing up of memory for IMA measurement >> logs. > > Last week, Mimi provided this feedback: > > "From your patch description, this patch should be broken up. Moving > the non-architecture specific code out of powerpc should be one patch. > Additional support should be in another patch. After each patch, the > code should work properly." > > That's not what you do here. You move the code, but you also make other > changes at the same time. This has two problems: > > 1. It makes the patch harder to review, because it's very easy to miss a > change. > > 2. If in the future a git bisect later points to this patch, it's not > clear whether the problem is because of the code movement, or because > of the other changes. > > When you move code, ideally the patch should only make the changes > necessary to make the code work at its new location. The patch which > does code movement should not cause any change in behavior. > > Other changes should go in separate patches, either before or after the > one moving the code. > > More comments below. > Hi Thiago, Apologies for the delayed response i was away for a few days. I am working on breaking up the changes so that its easier to review and update as well. Thanks, Prakhar Srivastava >> >> --- >> arch/powerpc/include/asm/ima.h | 10 --- >> arch/powerpc/kexec/ima.c | 126 ++--------------------------- >> security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++ >> 3 files changed, 124 insertions(+), 128 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h >> index ead488cf3981..c29ec86498f8 100644 >> --- a/arch/powerpc/include/asm/ima.h >> +++ b/arch/powerpc/include/asm/ima.h >> @@ -4,15 +4,6 @@ >> >> struct kimage; >> >> -int ima_get_kexec_buffer(void **addr, size_t *size); >> -int ima_free_kexec_buffer(void); >> - >> -#ifdef CONFIG_IMA >> -void remove_ima_buffer(void *fdt, int chosen_node); >> -#else >> -static inline void remove_ima_buffer(void *fdt, int chosen_node) {} >> -#endif >> - >> #ifdef CONFIG_IMA_KEXEC >> int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr, >> size_t size); >> @@ -22,7 +13,6 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node); >> static inline int setup_ima_buffer(const struct kimage *image, void *fdt, >> int chosen_node) >> { >> - remove_ima_buffer(fdt, chosen_node); >> return 0; >> } > > This is wrong. Even if the currently running kernel doesn't have > CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory > reservation from the FDT that is being prepared for the next kernel. > > This is because the IMA kexec buffer is useless for the next kernel, > regardless of whether the current kernel supports CONFIG_IMA_KEXEC or > not. Keeping it around would be a waste of memory. > I will keep it in my next revision. My understanding was the reserved memory is freed and property removed when IMA loads the logs on init. During setup_fdt in kexec, a duplicate copy of the dt is used, but memory still needs to be allocated, thus the property itself indicats presence of reserved memory. >> @@ -179,13 +64,18 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node) >> int ret, addr_cells, size_cells, entry_size; >> u8 value[16]; >> >> - remove_ima_buffer(fdt, chosen_node); > > This is wrong, for the same reason stated above. > >> if (!image->arch.ima_buffer_size) >> return 0; >> >> - ret = get_addr_size_cells(&addr_cells, &size_cells); >> - if (ret) >> + ret = fdt_address_cells(fdt, chosen_node); >> + if (ret < 0) >> + return ret; >> + addr_cells = ret; >> + >> + ret = fdt_size_cells(fdt, chosen_node); >> + if (ret < 0) >> return ret; >> + size_cells = ret; >> >> entry_size = 4 * (addr_cells + size_cells); >> > > I liked this change. Thanks! I agree it's better to use > fdt_address_cells() and fdt_size_cells() here. > > But it should be in a separate patch. Either before or after the one > moving the code. > >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 121de3e04af2..e1e6d6154015 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -10,8 +10,124 @@ >> #include >> #include >> #include >> +#include >> +#include >> +#include >> #include "ima.h" >> >> +static int get_addr_size_cells(int *addr_cells, int *size_cells) >> +{ >> + struct device_node *root; >> + >> + root = of_find_node_by_path("/"); >> + if (!root) >> + return -EINVAL; >> + >> + *addr_cells = of_n_addr_cells(root); >> + *size_cells = of_n_size_cells(root); >> + >> + of_node_put(root); >> + >> + return 0; >> +} >> + >> +static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr, >> + size_t *size) >> +{ >> + int ret, addr_cells, size_cells; >> + >> + ret = get_addr_size_cells(&addr_cells, &size_cells); >> + if (ret) >> + return ret; >> + >> + if (len < 4 * (addr_cells + size_cells)) >> + return -ENOENT; >> + >> + *addr = of_read_number(prop, addr_cells); >> + *size = of_read_number(prop + 4 * addr_cells, size_cells); >> + >> + return 0; >> +} >> + >> +/** >> + * ima_get_kexec_buffer - get IMA buffer from the previous kernel >> + * @addr: On successful return, set to point to the buffer contents. >> + * @size: On successful return, set to the buffer size. >> + * >> + * Return: 0 on success, negative errno on error. >> + */ >> +int ima_get_kexec_buffer(void **addr, size_t *size) >> +{ >> + int ret, len; >> + unsigned long tmp_addr; >> + size_t tmp_size; >> + const void *prop; >> + >> + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len); >> + if (!prop) >> + return -ENOENT; >> + >> + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size); >> + if (ret) >> + return ret; >> + >> + *addr = __va(tmp_addr); >> + *size = tmp_size; >> + >> + return 0; >> +} > > The functions above were moved without being changed. Good. > >> +/** >> + * ima_free_kexec_buffer - free memory used by the IMA buffer >> + */ >> +int ima_free_kexec_buffer(void) >> +{ >> + int ret; >> + unsigned long addr; >> + size_t size; >> + struct property *prop; >> + >> + prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL); >> + if (!prop) >> + return -ENOENT; >> + >> + ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size); >> + if (ret) >> + return ret; >> + >> + ret = of_remove_property(of_chosen, prop); >> + if (ret) >> + return ret; >> + >> + return memblock_free(__pa(addr), size); > > Here you added a __pa() call. Do you store a virtual address in > linux,ima-kexec-buffer property? Doesn't it make more sense to store a > physical address? > trying to minimize the changes here as do_get_kexec_buffer return the va. I will refactor this to remove the double translation. > Even if making this change is the correct thing to do, it should be a > separate patch, unless it can't be avoided. And if that is the case, > then it should be explained in the commit message. > >> + >> +} >> + >> +/** >> + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt >> + * >> + * The IMA measurement buffer is of no use to a subsequent kernel, so we always >> + * remove it from the device tree. >> + */ >> +void remove_ima_buffer(void *fdt, int chosen_node) >> +{ >> + int ret, len; >> + unsigned long addr; >> + size_t size; >> + const void *prop; >> + >> + prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len); >> + if (!prop) >> + return; >> + >> + do_get_kexec_buffer(prop, len, &addr, &size); >> + ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer"); >> + if (ret < 0) >> + return; >> + >> + memblock_free(addr, size); >> +} > > Here is another function that changed when moved. This one I know to be > wrong. You're confusing the purposes of remove_ima_buffer() and > ima_free_kexec_buffer(). > > You did send me a question about them nearly three weeks ago which I > only answered today, so I apologize. Also, their names could more > clearly reflect their differences, so it's bad naming on my part. > > With IMA kexec buffers, there are two kernels (and thus their two > respective, separate device trees) to be concerned about: > > 1. the currently running kernel, which uses the live device tree > (accessed with the of_* functions) and the memblock subsystem; > > 2. the kernel which is being loaded by kexec, which will use the FDT > blob being passed around as argument to these functions, and the memory > reservations in the memory reservation table of the FDT blob. > > ima_free_kexec_buffer() is used by IMA in the currently running kernel. > Therefore the device tree it is concerned about is the live one, and > thus uses the of_* functions to access it. And uses memblock to change > the memory reservation. > > remove_ima_buffer() on the other hand is used by the kexec code to > prepare an FDT blob for the kernel that is being loaded. It should not > make any changes to live device tree, nor to memblock allocations. It > should only make changes to the FDT blob. > Thank you for this, greatly appreciate clearing my misunderstandings. > -- > Thiago Jung Bauermann > IBM Linux Technology Center > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel