Re: [qemu-s390x] [PATCH] s390-bios: Skip bootmap signature entries

[Cornelia Huck]

On Mon, 6 May 2019 12:14:10 +0200
Christian Borntraeger <address@hidden> wrote:

> On 06.05.19 12:01, David Hildenbrand wrote:
> > On 29.04.19 15:09, Jason J. Herne wrote:  
> >> Newer versions of zipl have the ability to write signature entries to the 
> >> boot
> >> script for secure boot. We don't yet support secure boot, but we need to 
> >> skip
> >> over signature entries while reading the boot script in order to maintain 
> >> our
> >> ability to boot guest operating systems that have a secure bootloader.
> >>
> >> Signed-off-by: Jason J. Herne <address@hidden>
> >> Reviewed-by: Farhan Ali <address@hidden>
> >> ---
> >>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
> >>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
> >>  2 files changed, 23 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> >> index 7aef65a..d13b7cb 100644
> >> --- a/pc-bios/s390-ccw/bootmap.c
> >> +++ b/pc-bios/s390-ccw/bootmap.c
> >> @@ -254,7 +254,14 @@ static void run_eckd_boot_script(block_number_t 
> >> bmt_block_nr,
> >>      memset(sec, FREE_SPACE_FILLER, sizeof(sec));
> >>      read_block(block_nr, sec, "Cannot read Boot Map Script");
> >>  
> >> -    for (i = 0; bms->entry[i].type == BOOT_SCRIPT_LOAD; i++) {
> >> +    for (i = 0; bms->entry[i].type == BOOT_SCRIPT_LOAD ||
> >> +                bms->entry[i].type == BOOT_SCRIPT_SIGNATURE; i++) {
> >> +
> >> +        /* We don't support secure boot yet, so we skip signature entries 
> >> */
> >> +        if (bms->entry[i].type == BOOT_SCRIPT_SIGNATURE) {
> >> +            continue;
> >> +        }
> >> +
> >>          address = bms->entry[i].address.load_address;
> >>          block_nr = eckd_block_num(&bms->entry[i].blkptr.xeckd.bptr.chs);
> >>  
> >> @@ -489,7 +496,15 @@ static void zipl_run(ScsiBlockPtr *pte)
> >>  
> >>      /* Load image(s) into RAM */
> >>      entry = (ComponentEntry *)(&header[1]);
> >> -    while (entry->component_type == ZIPL_COMP_ENTRY_LOAD) {
> >> +    while (entry->component_type == ZIPL_COMP_ENTRY_LOAD ||
> >> +           entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> >> +
> >> +        /* We don't support secure boot yet, so we skip signature entries 
> >> */
> >> +        if (entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
> >> +            entry++;
> >> +            continue;
> >> +        }
> >> +
> >>          zipl_load_segment(entry);
> >>  
> >>          entry++;
> >> diff --git a/pc-bios/s390-ccw/bootmap.h b/pc-bios/s390-ccw/bootmap.h
> >> index a085212..94f53a5 100644
> >> --- a/pc-bios/s390-ccw/bootmap.h
> >> +++ b/pc-bios/s390-ccw/bootmap.h
> >> @@ -98,8 +98,9 @@ typedef struct ScsiMbr {
> >>  #define ZIPL_COMP_HEADER_IPL    0x00
> >>  #define ZIPL_COMP_HEADER_DUMP   0x01
> >>  
> >> -#define ZIPL_COMP_ENTRY_LOAD    0x02
> >> -#define ZIPL_COMP_ENTRY_EXEC    0x01
> >> +#define ZIPL_COMP_ENTRY_EXEC      0x01
> >> +#define ZIPL_COMP_ENTRY_LOAD      0x02
> >> +#define ZIPL_COMP_ENTRY_SIGNATURE 0x03
> >>  
> >>  typedef struct XEckdMbr {
> >>      uint8_t magic[4];   /* == "xIPL"        */
> >> @@ -117,8 +118,9 @@ typedef struct BootMapScriptEntry {
> >>      BootMapPointer blkptr;
> >>      uint8_t pad[7];
> >>      uint8_t type;   /* == BOOT_SCRIPT_* */
> >> -#define BOOT_SCRIPT_EXEC 0x01
> >> -#define BOOT_SCRIPT_LOAD 0x02
> >> +#define BOOT_SCRIPT_EXEC      0x01
> >> +#define BOOT_SCRIPT_LOAD      0x02
> >> +#define BOOT_SCRIPT_SIGNATURE 0x03
> >>      union {
> >>          uint64_t load_address;
> >>          uint64_t load_psw;
> >>  
> > 
> > Naive question from me:
> > 
> > Can't we place the signatures somewhere else, and instead associate them
> > with entries? This avoids breaking backwards compatibility for the sake
> > of signatures we want unmodified zipl loaders to ignore.
> >   
> 
> This way is according to hardware(or firmware) architecture for 
> list-directed IPL, so we have to live with it. In the end zipl can still
> write the old variant (without secure entries). The default (auto)
> will detect if the hardware supports secure IPL or not. (via 
> /sys/firmware/ipl/has_secure)
> So this toleration support here is necessary for things like installing in an
> LPAR that has secure boot and then IPLing that disk under a KVM that has not.

I'm a bit confused here. We want to tolerate booting from a boot record
that was written on an LPAR that supports secure IPL, which is
generally reasonable. But: Why is the boot record then written with or
without signature entries depending on the presence of the feature? How
'optional' are those entries, given they may be ignored for portability
reasons?