Re: [PATCH] pc-bios: s390x: Save iplb location in lowcore

From: David Hildenbrand <david@redhat.com>
To: Janosch Frank <frankja@linux.ibm.com>, qemu-devel@nongnu.org
Cc: borntraeger@de.ibm.com, qemu-s390x@nongnu.org, cohuck@redhat.com
Subject: Re: [PATCH] pc-bios: s390x: Save iplb location in lowcore
Date: Tue, 3 Mar 2020 17:13:36 +0100	[thread overview]
Message-ID: <49294c6a-38f6-8358-b613-72d3c3c7cf4f@redhat.com> (raw)
In-Reply-To: <20200303155010.2519-1-frankja@linux.ibm.com>

On 03.03.20 16:50, Janosch Frank wrote:
> The POP states that for a list directed IPL the IPLB is stored into
> memory by the machine loader and its address is stored at offset 0x14
> of the lowcore.
> 
> ZIPL currently uses the address in offset 0x14 to access the IPLB and
> acquire flags about secure boot. If the IPLB address points into
> memory which has an unsupported mix of flags set, ZIPL will panic
> instead of booting the OS.
> 
> As the lowcore can have quite a high entropy for a guest that did drop
> out of protected mode (i.e. rebooted) we encountered the ZIPL panic
> quite often.

How did this ever work? Or does this only become relevant with secure boot?

Fixes: ? Or has this always been broken?

> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com>
> ---
>  pc-bios/s390-ccw/jump2ipl.c  |  1 +
>  pc-bios/s390-ccw/main.c      |  8 +++++++-
>  pc-bios/s390-ccw/netmain.c   |  1 +
>  pc-bios/s390-ccw/s390-arch.h | 10 ++++++++--
>  pc-bios/s390-ccw/s390-ccw.h  |  1 +
>  5 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/pc-bios/s390-ccw/jump2ipl.c b/pc-bios/s390-ccw/jump2ipl.c
> index da13c43cc0..4eba2510b0 100644
> --- a/pc-bios/s390-ccw/jump2ipl.c
> +++ b/pc-bios/s390-ccw/jump2ipl.c
> @@ -35,6 +35,7 @@ void jump_to_IPL_code(uint64_t address)
>  {
>      /* store the subsystem information _after_ the bootmap was loaded */
>      write_subsystem_identification();
> +    write_iplb_location();
>  
>      /* prevent unknown IPL types in the guest */
>      if (iplb.pbt == S390_IPL_TYPE_QEMU_SCSI) {
> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
> index a21b386280..4e65b411e1 100644
> --- a/pc-bios/s390-ccw/main.c
> +++ b/pc-bios/s390-ccw/main.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "libc.h"
> +#include "helper.h"
>  #include "s390-arch.h"
>  #include "s390-ccw.h"
>  #include "cio.h"
> @@ -22,7 +23,7 @@ QemuIplParameters qipl;
>  IplParameterBlock iplb __attribute__((__aligned__(PAGE_SIZE)));
>  static bool have_iplb;
>  static uint16_t cutype;
> -LowCore const *lowcore; /* Yes, this *is* a pointer to address 0 */
> +LowCore *lowcore; /* Yes, this *is* a pointer to address 0 */
>  
>  #define LOADPARM_PROMPT "PROMPT  "
>  #define LOADPARM_EMPTY  "        "
> @@ -42,6 +43,11 @@ void write_subsystem_identification(void)
>      *zeroes = 0;
>  }
>  
> +void write_iplb_location(void)
> +{
> +    lowcore->ptr_iplb = ptr2u32(&iplb);
> +}
> +
>  void panic(const char *string)
>  {
>      sclp_print(string);
> diff --git a/pc-bios/s390-ccw/netmain.c b/pc-bios/s390-ccw/netmain.c
> index f2dcc01e27..309ffa30d9 100644
> --- a/pc-bios/s390-ccw/netmain.c
> +++ b/pc-bios/s390-ccw/netmain.c
> @@ -40,6 +40,7 @@
>  #define DEFAULT_TFTP_RETRIES 20
>  
>  extern char _start[];
> +void write_iplb_location(void) {}
>  
>  #define KERNEL_ADDR             ((void *)0L)
>  #define KERNEL_MAX_SIZE         ((long)_start)
> diff --git a/pc-bios/s390-ccw/s390-arch.h b/pc-bios/s390-ccw/s390-arch.h
> index 504fc7c2f0..5f36361c02 100644
> --- a/pc-bios/s390-ccw/s390-arch.h
> +++ b/pc-bios/s390-ccw/s390-arch.h
> @@ -36,7 +36,13 @@ typedef struct LowCore {
>      /* prefix area: defined by architecture */
>      PSWLegacy       ipl_psw;                  /* 0x000 */
>      uint32_t        ccw1[2];                  /* 0x008 */
> -    uint32_t        ccw2[2];                  /* 0x010 */
> +    union {
> +        uint32_t        ccw2[2];                  /* 0x010 */
> +        struct {
> +            uint32_t reserved10;
> +            uint32_t ptr_iplb;
> +        };
> +    };
>      uint8_t         pad1[0x80 - 0x18];        /* 0x018 */
>      uint32_t        ext_params;               /* 0x080 */
>      uint16_t        cpu_addr;                 /* 0x084 */
> @@ -85,7 +91,7 @@ typedef struct LowCore {
>      PSW             io_new_psw;               /* 0x1f0 */
>  } __attribute__((packed, aligned(8192))) LowCore;
>  
> -extern LowCore const *lowcore;
> +extern LowCore *lowcore;
>  
>  static inline void set_prefix(uint32_t address)
>  {
> diff --git a/pc-bios/s390-ccw/s390-ccw.h b/pc-bios/s390-ccw/s390-ccw.h
> index 11bce7d73c..21f27e7990 100644
> --- a/pc-bios/s390-ccw/s390-ccw.h
> +++ b/pc-bios/s390-ccw/s390-ccw.h
> @@ -57,6 +57,7 @@ void consume_io_int(void);
>  /* main.c */
>  void panic(const char *string);
>  void write_subsystem_identification(void);
> +void write_iplb_location(void);
>  extern char stack[PAGE_SIZE * 8] __attribute__((__aligned__(PAGE_SIZE)));
>  unsigned int get_loadparm_index(void);

In general LGTM.

Side note: I wonder if the assert in ptr2u32() should actually be

IPL_assert((uint64_t)ptr <= 0xfffffffful, "ptr2u32: ptr too large");
				      ^
or even better

IPL_assert((uint64_t)ptr != (uint32_t)ptr, "ptr2u32: ptr too large");

Otherwise, sign extension will simply always make this pass.

-- 
Thanks,

David / dhildenb