Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <f7df1bfc9a2ef4900dccb01ab81aa1fa9aaa6eb9.camel@linux.ibm.com>
Subject: Re: [PATCH v2] s390/ipl: fix virtual vs physical address confusion
From:   Mimi Zohar <zohar@linux.ibm.com>
To:     Alexander Gordeev <agordeev@linux.ibm.com>,
        Jarkko Sakkinen <jarkko@kernel.org>,
        Vasily Gorbik <gor@linux.ibm.com>,
        Heiko Carstens <hca@linux.ibm.com>
Cc:     linux-kernel@vger.kernel.org, linux-s390@vger.kernel.org,
        linux-integrity@vger.kernel.org
Date:   Wed, 16 Aug 2023 12:13:24 -0400
In-Reply-To: <20230816132942.2540411-1-agordeev@linux.ibm.com>
References: <20230816132942.2540411-1-agordeev@linux.ibm.com>
Content-Type: text/plain; charset="ISO-8859-15"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 2023-08-16 at 15:29 +0200, Alexander Gordeev wrote:
> The value of ipl_cert_list_addr boot variable contains
> a physical address, which is used directly. That works
> because virtual and physical address spaces are currently
> the same, but otherwise it is wrong.
> 
> While at it, fix also a comment for the platform keyring.
> 
> Signed-off-by: Alexander Gordeev <agordeev@linux.ibm.com>
> ---
>  arch/s390/kernel/machine_kexec_file.c             | 4 ++--
>  arch/s390/kernel/setup.c                          | 2 +-
>  security/integrity/platform_certs/load_ipl_s390.c | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
> index 2df94d32140c..8d207b82d9fe 100644
> --- a/arch/s390/kernel/machine_kexec_file.c
> +++ b/arch/s390/kernel/machine_kexec_file.c
> @@ -188,7 +188,7 @@ static int kexec_file_add_ipl_report(struct kimage *image,
>  	data->memsz = ALIGN(data->memsz, PAGE_SIZE);
>  	buf.mem = data->memsz;
>  
> -	ptr = (void *)ipl_cert_list_addr;
> +	ptr = __va(ipl_cert_list_addr);
>  	end = ptr + ipl_cert_list_size;
>  	ncerts = 0;
>  	while (ptr < end) {
> @@ -200,7 +200,7 @@ static int kexec_file_add_ipl_report(struct kimage *image,
>  
>  	addr = data->memsz + data->report->size;
>  	addr += ncerts * sizeof(struct ipl_rb_certificate_entry);
> -	ptr = (void *)ipl_cert_list_addr;
> +	ptr = __va(ipl_cert_list_addr);
>  	while (ptr < end) {
>  		len = *(unsigned int *)ptr;
>  		ptr += sizeof(len);
> diff --git a/arch/s390/kernel/setup.c b/arch/s390/kernel/setup.c
> index 393dd8385506..c744104e4a9c 100644
> --- a/arch/s390/kernel/setup.c
> +++ b/arch/s390/kernel/setup.c
> @@ -875,7 +875,7 @@ static void __init log_component_list(void)
>  		pr_info("Linux is running with Secure-IPL enabled\n");
>  	else
>  		pr_info("Linux is running with Secure-IPL disabled\n");
> -	ptr = (void *) early_ipl_comp_list_addr;
> +	ptr = __va(early_ipl_comp_list_addr);
>  	end = (void *) ptr + early_ipl_comp_list_size;
>  	pr_info("The IPL report contains the following components:\n");
>  	while (ptr < end) {
> diff --git a/security/integrity/platform_certs/load_ipl_s390.c b/security/integrity/platform_certs/load_ipl_s390.c
> index e769dcb7ea94..c7c381a9ddaa 100644
> --- a/security/integrity/platform_certs/load_ipl_s390.c
> +++ b/security/integrity/platform_certs/load_ipl_s390.c
> @@ -22,8 +22,8 @@ static int __init load_ipl_certs(void)
>  
>  	if (!ipl_cert_list_addr)
>  		return 0;
> -	/* Copy the certificates to the system keyring */
> -	ptr = (void *) ipl_cert_list_addr;
> +	/* Copy the certificates to the platform keyring */
> +	ptr = __va(ipl_cert_list_addr);
>  	end = ptr + ipl_cert_list_size;
>  	while ((void *) ptr < end) {
>  		len = *(unsigned int *) ptr;

ipl_cert_list_addr is defined as an unsigned long.  At this point, the
changes are simple cleanup of removing "(void *)" and replacing it with
__va().

From arch/s390/include/asm/page.h:
#define __pa(x)                 ((unsigned long)(x))
#define __va(x)                 ((void *)(unsigned long)(x))

So, Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

I'm trying to understand if there is a difference between the other
archs and s390; and whether a similar change is needed for the other
archs.  Loading certificates on the other archs call kmalloc to
allocate memory for the certs. Is the memory being allocated on x390
using kmalloc?

-- 
thanks,

Mimi