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

[Janosch Frank]

On 3/3/20 5:13 PM, David Hildenbrand wrote:
> 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?

I'd guess that until secure boot ZIPL never touched this and with it we
never hit the right combination of flags to trigger a ZIPL panic.

This way of getting to the IPLB was used before diag308 was available,
i.e. way before KVM got to IBM Z. It looks like ZIPL only uses it for
secure boot for some reason and hence we never implemented it before.

I'm also in discussion with the ZIPL developers to make this more robust.

> 
> Fixes: ? Or has this always been broken?

See above

> 
>>
>> Signed-off-by: Janosch Frank <address@hidden>
>> Tested-by: Marc Hartmayer <address@hidden>
>> ---
>>  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.
> 

Do you want to add a patch or shall I add it to my cleanup series?

signature.asc
Description: OpenPGP digital signature