Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jonathan McDowell <noodles@fb.com>
To:     Stefan Berger <stefanb@linux.ibm.com>
CC:     Mimi Zohar <zohar@linux.ibm.com>,
        "kexec@lists.infradead.org" <kexec@lists.infradead.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "nayna@linux.ibm.com" <nayna@linux.ibm.com>,
        "nasastry@in.ibm.com" <nasastry@in.ibm.com>,
        "mpe@ellerman.id.au" <mpe@ellerman.id.au>,
        Rob Herring <robh+dt@kernel.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Borislav Petkov <bp@suse.de>
Subject: Re: [PATCH v4 4/5] of: kexec: Refactor IMA buffer related functions
 to make them reusable
Thread-Topic: [PATCH v4 4/5] of: kexec: Refactor IMA buffer related functions
 to make them reusable
Thread-Index: AQHYkMErYgOTAJp330m4YH5UOsaqI61xX/2A
Date:   Wed, 6 Jul 2022 14:00:44 +0000
Message-ID: <YsWVfbMu85Cmwdgm@noodles-fedora.dhcp.thefacebook.com>
References: <20220701022603.31076-1-stefanb@linux.ibm.com>
 <20220701022603.31076-5-stefanb@linux.ibm.com>
 <47256afac54d68c23f0bdec257ffa26ddf1eb25d.camel@linux.ibm.com>
In-Reply-To: <47256afac54d68c23f0bdec257ffa26ddf1eb25d.camel@linux.ibm.com>
Accept-Language: en-GB, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?/UdPMDwGKEYkMNA7yigW0x8DH+47RrCl0eKjmIptLcSjyyv+ez2DhBp1nOam?=
 =?us-ascii?Q?puBbbDGDdlz5lP6BKJMeuwbJa4YDYohwgUCRntkysFLLb55ELexK1Yj/0Rx5?=
 =?us-ascii?Q?7CFu1e9c1fnSrsOkc+5LxPevKcB3fZi8T+R8jOgTa7VYtcA1bpoqCMZQgq4e?=
 =?us-ascii?Q?YXOzd/fvuZE6D/udCG83ppg4WGxyg/kw5BO9TYgo7dLwxAhq9fUQ+9hgqZRH?=
 =?us-ascii?Q?O3sY0cq1Mg7fzGtLexFzt6nYH+U1sJrJX9E23Q/jY2xUZi92S/PLqemnOHW2?=
 =?us-ascii?Q?JD4kZsEVthJgLMDpjK9JqQaqh6MKb3HfhzIpmMv9NvdT43KSRKDXIycHE8q9?=
 =?us-ascii?Q?Wcj6NsAKHW38uNcgbRrTcDms/43HDCStr3o7sesSkRY0a244Kjkp4ZrzUuJZ?=
 =?us-ascii?Q?hpMMwMRpAUGvUOLT5jezUKFFloeIOUJIiQMo2baLYcGL4usubFik/E/cWnzf?=
 =?us-ascii?Q?RmHSlSy8qNYezu1MxGGGMz+CxSidvFFSQ4a7v1br4ksCVHHdYZ50e4sPDBGy?=
 =?us-ascii?Q?DE9eDdmU3/x7VcORQNcdFVGg7ZLwcrDh/ZQHHWx5impZPNRgvaZlK/XaTKii?=
 =?us-ascii?Q?9GO1r4dPkJjkWkSQy9tEmPLc/G6VHXdL5qmaqp5Ckh3XMNrk+gl9+mN9YbYJ?=
 =?us-ascii?Q?pXEquMRGLpy5kjw+THT7SOBBj2IyLTPXavJNodd3R7fCYqtNJdvXSvRqlvKB?=
 =?us-ascii?Q?y/TxNlnw7+pmvP0UoKk3Vb1Q3o1PipMiY+sZ8kGs4FRyJzZUiXVHqL9Kau7Z?=
 =?us-ascii?Q?AJ1YB9cv+4nV82tolaCcAr2KOQSRzFIrifXPsZlqvRcArZwhxwDzuWlnAkha?=
 =?us-ascii?Q?ig4euM0RaBeMt7S2dsd/ju24d8fuupcxVs4zqOPBPFZB/urhMGUDz0Ld2jtm?=
 =?us-ascii?Q?VjAA3ZfuvwwSL3husN5+6/fL4bWF90zVLhxS0fYxfaOodWTB4eYVykM3UxJd?=
 =?us-ascii?Q?R8T0p4dlaLgG6jvzdK7MpqFU3D64JMl2M8FIZ4Vke00y5cuXamcUZAidzrw9?=
 =?us-ascii?Q?ojCDu/VNvelYeqfN492zf3VMTjpGj8lGz3l9nF+Uc2u3MNYR4F98boXKV8KK?=
 =?us-ascii?Q?vNpdXMrebgxqNIxtl8QBsxxBvXrJnQ5YPm/5PVSAXqzYLgc6+wfmLqjtSZhb?=
 =?us-ascii?Q?IX+hLuXBwH5nrkuGQL6nk1/Rph0/4rMlr33vhSHTUhPAHXVXgv8o/mhcE/Y+?=
 =?us-ascii?Q?VCDALnYekvFuaBd51T127AonI2aor18lKBK7bWqx+hiH1nOqJDXKIEp3aJcn?=
 =?us-ascii?Q?BWXL5lMXhKYXFehy6vOvZSRTEyQB59A6vUGGuNhOYgZO2dfHpsFSn020Lk02?=
 =?us-ascii?Q?2Qhj+ZMOMuSzKMf8ERdHmXYmNlgVjO0UVZfGTPj8qix5ga3rzMiuqqbPYAD/?=
 =?us-ascii?Q?BCp0LRAoa6ZuiO6x7F2TMiA7p+j9UTuVGYll+BURkW4tPK9wVeuAQU9GQnBE?=
 =?us-ascii?Q?lGIpPm3Ndd4jIt1JbtmeH1eTzeTd+PTsqv2Gl21H41ziVwDYhzuWoU0nkcq/?=
 =?us-ascii?Q?gfIrvYVzpE2ALAoAxSv0JiJ4tPdmpi+ONpGrEu4BsUPw253HcSgTDO9ivaUd?=
 =?us-ascii?Q?AGlem5+/D54Mm8rzKVogXiC8L/5jR71Vm2yWKfkx?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3DCCC94782084E419D9F1409E28AFED5@namprd15.prod.outlook.com>
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR15MB4552.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 2eeb2b21-33ec-4b47-df1a-08da5f57eb6f
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jul 2022 14:00:44.2492
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 4DGpyowW2tvBdg578hLVwdqsJLWHVFveRKo1gL4GfKt/PSNelpiFTOAXcQxJaEBT
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR15MB5071
X-Proofpoint-GUID: EoiMq6u3Dk63jWqdXXnSDzOSAKy6eh5m
X-Proofpoint-ORIG-GUID: EoiMq6u3Dk63jWqdXXnSDzOSAKy6eh5m
X-Proofpoint-UnRewURL: 0 URL was un-rewritten
MIME-Version: 1.0
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-06_08,2022-06-28_01,2022-06-22_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jul 05, 2022 at 06:46:54PM -0400, Mimi Zohar wrote:
> [Cc'ing Borislav Petkov <bp@suse.de>, Jonathan McDowell <noodles@fb.com
> >]
> 
> Hi Stefan,
> 
> On Thu, 2022-06-30 at 22:26 -0400, Stefan Berger wrote:
> > Refactor IMA buffer related functions to make them reusable for carrying
> > TPM logs across kexec.
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Frank Rowand <frowand.list@gmail.com>
> > Cc: Mimi Zohar <zohar@linux.ibm.com>
> 
> Refactoring the ima_get_kexec_buffer sounds good, but there's a merge
> conflict with Jonathan McDowell's commit "b69a2afd5afc x86/kexec: Carry
> forward IMA measurement log on kexec".
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/of/kexec.c

None of this looks difficult to re-do on top of my changes that are in
-next; the only thing to watch out for is a couple of functions have
moved into the __init section but that looks appropriate for your TPM
log carry-over too.

> > ---
> > v4:
> >  - Move debug output into setup_buffer()
> > ---
> >  drivers/of/kexec.c | 131 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 76 insertions(+), 55 deletions(-)
> > 
> > diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> > index c4f9b6655a2e..0710703acfb0 100644
> > --- a/drivers/of/kexec.c
> > +++ b/drivers/of/kexec.c
> > @@ -115,48 +115,59 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
> >  	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)
> > +static int get_kexec_buffer(const char *name, unsigned long *addr, size_t *size)
> >  {
> >  	int ret, len;
> > -	unsigned long tmp_addr;
> >  	unsigned long start_pfn, end_pfn;
> > -	size_t tmp_size;
> >  	const void *prop;
> >  
> > -	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> > -		return -ENOTSUPP;
> > -
> > -	prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len);
> > +	prop = of_get_property(of_chosen, name, &len);
> >  	if (!prop)
> >  		return -ENOENT;
> >  
> > -	ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
> > +	ret = do_get_kexec_buffer(prop, len, addr, size);
> >  	if (ret)
> >  		return ret;
> >  
> > -	/* Do some sanity on the returned size for the ima-kexec buffer */
> > -	if (!tmp_size)
> > +	/* Do some sanity on the returned size for the kexec buffer */
> > +	if (!*size)
> >  		return -ENOENT;
> >  
> >  	/*
> >  	 * Calculate the PFNs for the buffer and ensure
> >  	 * they are with in addressable memory.
> >  	 */
> > -	start_pfn = PHYS_PFN(tmp_addr);
> > -	end_pfn = PHYS_PFN(tmp_addr + tmp_size - 1);
> > +	start_pfn = PHYS_PFN(*addr);
> > +	end_pfn = PHYS_PFN(*addr + *size - 1);
> >  	if (!page_is_ram(start_pfn) || !page_is_ram(end_pfn)) {
> > -		pr_warn("IMA buffer at 0x%lx, size = 0x%zx beyond memory\n",
> > -			tmp_addr, tmp_size);
> > +		pr_warn("%s buffer at 0x%lx, size = 0x%zx beyond memory\n",
> > +			name, *addr, *size);
> >  		return -EINVAL;
> >  	}
> >  
> > +	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;
> > +	unsigned long tmp_addr;
> > +	size_t tmp_size;
> > +
> > +	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> > +		return -ENOTSUPP;
> > +
> > +	ret = get_kexec_buffer("linux,ima-kexec-buffer", &tmp_addr, &tmp_size);
> > +	if (ret)
> > +		return ret;
> > +
> >  	*addr = __va(tmp_addr);
> >  	*size = tmp_size;
> >  
> > @@ -191,72 +202,82 @@ int ima_free_kexec_buffer(void)
> >  	return memblock_phys_free(addr, size);
> >  }
> >  
> > -/**
> > - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> > - *
> > - * @fdt: Flattened Device Tree to update
> > - * @chosen_node: Offset to the chosen node in the device tree
> > - *
> > - * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> > - * remove it from the device tree.
> > - */
> > -static void remove_ima_buffer(void *fdt, int chosen_node)
> > +static int remove_buffer(void *fdt, int chosen_node, const char *name)
> >  {
> >  	int ret, len;
> >  	unsigned long addr;
> >  	size_t size;
> >  	const void *prop;
> >  
> > -	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> > -		return;
> > -
> > -	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
> > +	prop = fdt_getprop(fdt, chosen_node, name, &len);
> >  	if (!prop)
> > -		return;
> > +		return -ENOENT;
> >  
> >  	ret = do_get_kexec_buffer(prop, len, &addr, &size);
> > -	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> > +	fdt_delprop(fdt, chosen_node, name);
> >  	if (ret)
> > -		return;
> > +		return ret;
> >  
> >  	ret = fdt_find_and_del_mem_rsv(fdt, addr, size);
> >  	if (!ret)
> > -		pr_debug("Removed old IMA buffer reservation.\n");
> > +		pr_debug("Remove old %s buffer reserveration", name);
> > +	return ret;
> >  }
> >  
> > -#ifdef CONFIG_IMA_KEXEC
> >  /**
> > - * setup_ima_buffer - add IMA buffer information to the fdt
> > - * @image:		kexec image being loaded.
> > - * @fdt:		Flattened device tree for the next kernel.
> > - * @chosen_node:	Offset to the chosen node.
> > + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> >   *
> > - * Return: 0 on success, or negative errno on error.
> > + * @fdt: Flattened Device Tree to update
> > + * @chosen_node: Offset to the chosen node in the device tree
> > + *
> > + * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> > + * remove it from the device tree.
> >   */
> > -static int setup_ima_buffer(const struct kimage *image, void *fdt,
> > -			    int chosen_node)
> > +static void remove_ima_buffer(void *fdt, int chosen_node)
> > +{
> > +	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> > +		return;
> > +
> > +	remove_buffer(fdt, chosen_node, "linux,ima-kexec-buffer");
> > +}
> > +
> > +#ifdef CONFIG_IMA_KEXEC
> > +static int setup_buffer(void *fdt, int chosen_node, const char *name,
> > +			phys_addr_t addr, size_t size)
> >  {
> >  	int ret;
> >  
> > -	if (!image->ima_buffer_size)
> > +	if (!size)
> >  		return 0;
> >  
> >  	ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
> > -				       "linux,ima-kexec-buffer",
> > -				       image->ima_buffer_addr,
> > -				       image->ima_buffer_size);
> > +				       name, addr, size);
> >  	if (ret < 0)
> >  		return -EINVAL;
> >  
> > -	ret = fdt_add_mem_rsv(fdt, image->ima_buffer_addr,
> > -			      image->ima_buffer_size);
> > +	ret = fdt_add_mem_rsv(fdt, addr, size);
> >  	if (ret)
> >  		return -EINVAL;
> >  
> > -	pr_debug("IMA buffer at 0x%pa, size = 0x%zx\n",
> > -		 &image->ima_buffer_addr, image->ima_buffer_size);
> > +	pr_debug("%s at 0x%pa, size = 0x%zx\n", name, &addr, size);
> >  
> >  	return 0;
> > +
> > +}
> > +
> > +/**
> > + * setup_ima_buffer - add IMA buffer information to the fdt
> > + * @image:		kexec image being loaded.
> > + * @fdt:		Flattened device tree for the next kernel.
> > + * @chosen_node:	Offset to the chosen node.
> > + *
> > + * Return: 0 on success, or negative errno on error.
> > + */
> > +static int setup_ima_buffer(const struct kimage *image, void *fdt,
> > +			    int chosen_node)
> > +{
> > +	return setup_buffer(fdt, chosen_node, "linux,ima-kexec-buffer",
> > +			    image->ima_buffer_addr, image->ima_buffer_size);
> >  }
> >  #else /* CONFIG_IMA_KEXEC */
> >  static inline int setup_ima_buffer(const struct kimage *image, void *fdt,