[v2] s390/ipl: fix virtual vs physical address confusion

[PATCH v2] s390/ipl: fix virtual vs physical address confusion

Posted by Alexander Gordeev 2 years ago

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;
-- 
2.39.2

Re: [PATCH v2] s390/ipl: fix virtual vs physical address confusion

Posted by Mimi Zohar 2 years ago

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

Re: [PATCH v2] s390/ipl: fix virtual vs physical address confusion

Posted by Alexander Gordeev 2 years ago

On Wed, Aug 16, 2023 at 12:13:24PM -0400, Mimi Zohar wrote:
...
> 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?

No. The memory is allocated in the decompressor and passed to
the uncompressed kernel. I do not think anything needs to be
done for other archs.

> -- 
> thanks,
> 
> Mimi

Thanks!

Re: [PATCH v2] s390/ipl: fix virtual vs physical address confusion

Posted by Heiko Carstens 2 years ago

On Wed, Aug 16, 2023 at 03:29:42PM +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(-)

Mimi, Jarkko, any objections from your side?
I would take this via the s390 tree.

Re: [PATCH v2] s390/ipl: fix virtual vs physical address confusion

Posted by Jarkko Sakkinen 2 years ago

On Wed Aug 16, 2023 at 6:44 PM EEST, Heiko Carstens wrote:
> On Wed, Aug 16, 2023 at 03:29:42PM +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(-)
>
> Mimi, Jarkko, any objections from your side?
> I would take this via the s390 tree.

No objections.

Acked-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH v2] s390/ipl: fix virtual vs physical address confusion

Posted by Mimi Zohar 2 years ago

On Wed, 2023-08-16 at 17:44 +0200, Heiko Carstens wrote:
> On Wed, Aug 16, 2023 at 03:29:42PM +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(-)
> 
> Mimi, Jarkko, any objections from your side?
> I would take this via the s390 tree.

No objections from my side.

-- 
thanks,

Mimi