s390-bios: Skip bootmap signature entries

Commit Message

Jason J. Herne April 29, 2019, 1:09 p.m. UTC

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 <[e-mail]>
Reviewed-by: Farhan Ali <[e-mail]>
---
 pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
 pc-bios/s390-ccw/bootmap.h | 10 ++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

Comments

Cornelia Huck April 29, 2019, 1:40 p.m. UTC | #1

On Mon, 29 Apr 2019 09:09:41 -0400
"Jason J. Herne" <[e-mail]> 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 <[e-mail]>
> Reviewed-by: Farhan Ali <[e-mail]>
> ---
>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
>  2 files changed, 23 insertions(+), 6 deletions(-)

Skipping something that we don't have support for yet, but that doesn't
hinder normal operation sounds reasonable; but can you point me to the
relevant commit implementing this in zipl or some documentation? I
haven't been able to find something like that in the s390-tools git
tree.

Christian Borntraeger April 29, 2019, 1:45 p.m. UTC | #2

On 29.04.19 15:40, Cornelia Huck wrote:
> On Mon, 29 Apr 2019 09:09:41 -0400
> "Jason J. Herne" <[e-mail]> 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 <[e-mail]>
>> Reviewed-by: Farhan Ali <[e-mail]>
>> ---
>>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
>>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
>>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> Skipping something that we don't have support for yet, but that doesn't
> hinder normal operation sounds reasonable; but can you point me to the
> relevant commit implementing this in zipl or some documentation? I
> haven't been able to find something like that in the s390-tools git
> tree.

the s390-tools update is currently in preparation, adding Stefan and Peter.

Peter Oberparleiter April 30, 2019, 9:24 a.m. UTC | #3

On 29.04.2019 15:45, Christian Borntraeger wrote:
> 
> 
> On 29.04.19 15:40, Cornelia Huck wrote:
>> On Mon, 29 Apr 2019 09:09:41 -0400
>> "Jason J. Herne" <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Farhan Ali <[e-mail]>
>>> ---
>>>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
>>>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
>>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> Skipping something that we don't have support for yet, but that doesn't
>> hinder normal operation sounds reasonable; but can you point me to the
>> relevant commit implementing this in zipl or some documentation? I
>> haven't been able to find something like that in the s390-tools git
>> tree.
> 
> the s390-tools update is currently in preparation, adding Stefan and Peter.

Here's the link to the s390-tools commit that introduces the new
signature component to zipl:

https://github.com/ibm-s390-tools/s390-tools/commit/7c7e10ed8fb048efc4e0cd91b0f6fa704fba128e

Cornelia Huck April 30, 2019, 9:44 a.m. UTC | #4

On Tue, 30 Apr 2019 11:24:27 +0200
Peter Oberparleiter <[e-mail]> wrote:

> On 29.04.2019 15:45, Christian Borntraeger wrote:
> > 
> > 
> > On 29.04.19 15:40, Cornelia Huck wrote:  
> >> On Mon, 29 Apr 2019 09:09:41 -0400
> >> "Jason J. Herne" <[e-mail]> 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 <[e-mail]>
> >>> Reviewed-by: Farhan Ali <[e-mail]>
> >>> ---
> >>>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
> >>>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
> >>>  2 files changed, 23 insertions(+), 6 deletions(-)  
> >>
> >> Skipping something that we don't have support for yet, but that doesn't
> >> hinder normal operation sounds reasonable; but can you point me to the
> >> relevant commit implementing this in zipl or some documentation? I
> >> haven't been able to find something like that in the s390-tools git
> >> tree.  
> > 
> > the s390-tools update is currently in preparation, adding Stefan and Peter.  
> 
> Here's the link to the s390-tools commit that introduces the new
> signature component to zipl:
> 
> https://github.com/ibm-s390-tools/s390-tools/commit/7c7e10ed8fb048efc4e0cd91b0f6fa704fba128e
> 
> 

Thanks!

Thomas Huth May 3, 2019, 9:34 a.m. UTC | #5

On 29/04/2019 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 <[e-mail]>
> Reviewed-by: Farhan Ali <[e-mail]>
> ---
>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
>  2 files changed, 23 insertions(+), 6 deletions(-)

Acked-by: Thomas Huth <[e-mail]>

Jason, Christian, do you have any other changes to the s390-ccw firmware
currently pending that you plan to send out within the next week? If so,
I'll wait a little bit longer with building a new binary for the
repository ... if not, I'll pick this patch up next week.

 Thomas

Christian Borntraeger May 6, 2019, 8:08 a.m. UTC | #6

On 03.05.19 11:34, Thomas Huth wrote:
> On 29/04/2019 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 <[e-mail]>
>> Reviewed-by: Farhan Ali <[e-mail]>
>> ---
>>  pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
>>  pc-bios/s390-ccw/bootmap.h | 10 ++++++----
>>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> Acked-by: Thomas Huth <[e-mail]>
> 
> Jason, Christian, do you have any other changes to the s390-ccw firmware
> currently pending that you plan to send out within the next week? If so,
> I'll wait a little bit longer with building a new binary for the
> repository ... if not, I'll pick this patch up next week.


I have nothing at the moment.

David Hildenbrand May 6, 2019, 10:01 a.m. UTC | #7

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 <[e-mail]>
> Reviewed-by: Farhan Ali <[e-mail]>
> ---
>  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.

David Hildenbrand May 6, 2019, 10:10 a.m. UTC | #8

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 <[e-mail]>
>> Reviewed-by: Farhan Ali <[e-mail]>
>> ---
>>  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.
> 


... but I guess this is already documented somewhere internally and
other components have been adjusted. IOW, cannot be changed anymore.

Guess our implementation should have tolerated other entries than
"BOOT_SCRIPT_LOAD" right from the beginning.

Christian Borntraeger May 6, 2019, 10:14 a.m. UTC | #9

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 <[e-mail]>
>> Reviewed-by: Farhan Ali <[e-mail]>
>> ---
>>  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.

Thomas Huth May 6, 2019, 10:16 a.m. UTC | #10

On 06/05/2019 12.10, David Hildenbrand 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 <[e-mail]>
>>> Reviewed-by: Farhan Ali <[e-mail]>
>>> ---
>>>  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.
>>
> 
> 
> ... but I guess this is already documented somewhere internally and
> other components have been adjusted. IOW, cannot be changed anymore.
> 
> Guess our implementation should have tolerated other entries than
> "BOOT_SCRIPT_LOAD" right from the beginning.

Hmm, now we only tolerate the _LOAD and _SIGNATURE entries, but still
nothing else... would it make sense to rewrite the code a little bit to
tolerate all other kind of entries, but just act on the well-known _LOAD
entries, so that we do not step into this trap in the future anymore?

 Thomas

Christian Borntraeger May 6, 2019, 10:18 a.m. UTC | #11

On 06.05.19 12:16, Thomas Huth wrote:
> On 06/05/2019 12.10, David Hildenbrand 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 <[e-mail]>
>>>> Reviewed-by: Farhan Ali <[e-mail]>
>>>> ---
>>>>  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.
>>>
>>
>>
>> ... but I guess this is already documented somewhere internally and
>> other components have been adjusted. IOW, cannot be changed anymore.
>>
>> Guess our implementation should have tolerated other entries than
>> "BOOT_SCRIPT_LOAD" right from the beginning.
> 
> Hmm, now we only tolerate the _LOAD and _SIGNATURE entries, but still
> nothing else... would it make sense to rewrite the code a little bit to
> tolerate all other kind of entries, but just act on the well-known _LOAD
> entries, so that we do not step into this trap in the future anymore?

I think we should not. Those entries might have sematic elements that the guest
wants to enforce. I do not think that this will come, but imagine a boot entry
that mandates some security wishes (e.g. do only run on non-shared cores).

Cornelia Huck May 6, 2019, 10:30 a.m. UTC | #12

On Mon, 6 May 2019 12:14:10 +0200
Christian Borntraeger <[e-mail]> 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 <[e-mail]>
> >> Reviewed-by: Farhan Ali <[e-mail]>
> >> ---
> >>  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?

Cornelia Huck May 6, 2019, 10:34 a.m. UTC | #13

On Mon, 6 May 2019 12:18:42 +0200
Christian Borntraeger <[e-mail]> wrote:

> On 06.05.19 12:16, Thomas Huth wrote:
> > On 06/05/2019 12.10, David Hildenbrand 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 <[e-mail]>
> >>>> Reviewed-by: Farhan Ali <[e-mail]>
> >>>> ---
> >>>>  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.
> >>>  
> >>
> >>
> >> ... but I guess this is already documented somewhere internally and
> >> other components have been adjusted. IOW, cannot be changed anymore.
> >>
> >> Guess our implementation should have tolerated other entries than
> >> "BOOT_SCRIPT_LOAD" right from the beginning.  
> > 
> > Hmm, now we only tolerate the _LOAD and _SIGNATURE entries, but still
> > nothing else... would it make sense to rewrite the code a little bit to
> > tolerate all other kind of entries, but just act on the well-known _LOAD
> > entries, so that we do not step into this trap in the future anymore?  
> 
> I think we should not. Those entries might have sematic elements that the guest
> wants to enforce. I do not think that this will come, but imagine a boot entry
> that mandates some security wishes (e.g. do only run on non-shared cores).

Can we split the namespace for BOOT_SCRIPT into 'ignore if you don't
know what that is' and 'fail if you don't know what that is'? I'm
completely confused how 'optional' those entries are supposed to be...

Christian Borntraeger May 6, 2019, 10:45 a.m. UTC | #14

On 06.05.19 12:30, Cornelia Huck wrote:
> On Mon, 6 May 2019 12:14:10 +0200
> Christian Borntraeger <[e-mail]> 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 <[e-mail]>
>>>> Reviewed-by: Farhan Ali <[e-mail]>
>>>> ---
>>>>  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?

zipl does not write those blocks on systems that do will not be able to handle
those. This is something that I asked for. This will make the common use case
still working fine with all KVM systems. (Since KVM parses the zipl layout).

We now still have cases where there are these signature entries. Ignoring
those is also fine, since these records are only relevant if the LPAR is 
enable for secure boot. It is somewhat like on a windows PC. The windows is 
installed with everything necessary to do the secure boot. If you disable
secure boot in the bios, the signatures are just ignored.  (minus Windows 8.1
which showed a watermark on the desktop. Microsoft provided a patch to
disable that watermark).

Makes sense?

Christian Borntraeger May 6, 2019, 10:46 a.m. UTC | #15

On 06.05.19 12:34, Cornelia Huck wrote:
> On Mon, 6 May 2019 12:18:42 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 06.05.19 12:16, Thomas Huth wrote:
>>> On 06/05/2019 12.10, David Hildenbrand 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 <[e-mail]>
>>>>>> Reviewed-by: Farhan Ali <[e-mail]>
>>>>>> ---
>>>>>>  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.
>>>>>  
>>>>
>>>>
>>>> ... but I guess this is already documented somewhere internally and
>>>> other components have been adjusted. IOW, cannot be changed anymore.
>>>>
>>>> Guess our implementation should have tolerated other entries than
>>>> "BOOT_SCRIPT_LOAD" right from the beginning.  
>>>
>>> Hmm, now we only tolerate the _LOAD and _SIGNATURE entries, but still
>>> nothing else... would it make sense to rewrite the code a little bit to
>>> tolerate all other kind of entries, but just act on the well-known _LOAD
>>> entries, so that we do not step into this trap in the future anymore?  
>>
>> I think we should not. Those entries might have sematic elements that the guest
>> wants to enforce. I do not think that this will come, but imagine a boot entry
>> that mandates some security wishes (e.g. do only run on non-shared cores).
> 
> Can we split the namespace for BOOT_SCRIPT into 'ignore if you don't
> know what that is' and 'fail if you don't know what that is'? I'm
> completely confused how 'optional' those entries are supposed to be...

Since we do not know if and what future entries will come the current default
of failing seems the best approach. We can then add things to pc-bios when
necessary.

Cornelia Huck May 6, 2019, 11:05 a.m. UTC | #16

On Mon, 6 May 2019 12:46:50 +0200
Christian Borntraeger <[e-mail]> wrote:

> On 06.05.19 12:34, Cornelia Huck wrote:
> > On Mon, 6 May 2019 12:18:42 +0200
> > Christian Borntraeger <[e-mail]> wrote:

> >> I think we should not. Those entries might have sematic elements that the guest
> >> wants to enforce. I do not think that this will come, but imagine a boot entry
> >> that mandates some security wishes (e.g. do only run on non-shared cores).  
> > 
> > Can we split the namespace for BOOT_SCRIPT into 'ignore if you don't
> > know what that is' and 'fail if you don't know what that is'? I'm
> > completely confused how 'optional' those entries are supposed to be...  
> 
> Since we do not know if and what future entries will come the current default
> of failing seems the best approach. We can then add things to pc-bios when
> necessary.

That's where I'm coming from: Have some values where unknown entries
lead to (desired) failure, and others where unknown entries are simply
ignored. That would give us automatic toleration for optional entries.

Christian Borntraeger May 6, 2019, 11:13 a.m. UTC | #17

On 06.05.19 13:05, Cornelia Huck wrote:
> On Mon, 6 May 2019 12:46:50 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 06.05.19 12:34, Cornelia Huck wrote:
>>> On Mon, 6 May 2019 12:18:42 +0200
>>> Christian Borntraeger <[e-mail]> wrote:
> 
>>>> I think we should not. Those entries might have sematic elements that the guest
>>>> wants to enforce. I do not think that this will come, but imagine a boot entry
>>>> that mandates some security wishes (e.g. do only run on non-shared cores).  
>>>
>>> Can we split the namespace for BOOT_SCRIPT into 'ignore if you don't
>>> know what that is' and 'fail if you don't know what that is'? I'm
>>> completely confused how 'optional' those entries are supposed to be...  
>>
>> Since we do not know if and what future entries will come the current default
>> of failing seems the best approach. We can then add things to pc-bios when
>> necessary.
> 
> That's where I'm coming from: Have some values where unknown entries
> lead to (desired) failure, and others where unknown entries are simply
> ignored. That would give us automatic toleration for optional entries.

Well, this is the first new entry after 14 years of list-directed-ipl so there
is a slight chance to over-engineer here ;-)

In the end this is a field that does not belong to Linux-only, it is also defined
by the machine architecture.

Cornelia Huck May 6, 2019, 11:23 a.m. UTC | #18

On Mon, 6 May 2019 13:13:55 +0200
Christian Borntraeger <[e-mail]> wrote:

> On 06.05.19 13:05, Cornelia Huck wrote:
> > On Mon, 6 May 2019 12:46:50 +0200
> > Christian Borntraeger <[e-mail]> wrote:
> >   
> >> On 06.05.19 12:34, Cornelia Huck wrote:  
> >>> On Mon, 6 May 2019 12:18:42 +0200
> >>> Christian Borntraeger <[e-mail]> wrote:  
> >   
> >>>> I think we should not. Those entries might have sematic elements that the guest
> >>>> wants to enforce. I do not think that this will come, but imagine a boot entry
> >>>> that mandates some security wishes (e.g. do only run on non-shared cores).    
> >>>
> >>> Can we split the namespace for BOOT_SCRIPT into 'ignore if you don't
> >>> know what that is' and 'fail if you don't know what that is'? I'm
> >>> completely confused how 'optional' those entries are supposed to be...    
> >>
> >> Since we do not know if and what future entries will come the current default
> >> of failing seems the best approach. We can then add things to pc-bios when
> >> necessary.  
> > 
> > That's where I'm coming from: Have some values where unknown entries
> > lead to (desired) failure, and others where unknown entries are simply
> > ignored. That would give us automatic toleration for optional entries.  
> 
> Well, this is the first new entry after 14 years of list-directed-ipl so there
> is a slight chance to over-engineer here ;-)
> 
> In the end this is a field that does not belong to Linux-only, it is also defined
> by the machine architecture.

Yeah, I understand that having to get this into the main architecture
makes this harder to change.

If there is nothing coming in the foreseeable future that would need
toleration (and not failure), it's probably not worth spending more
time on that and we should just go with this patch.

I'd recommend putting this (+ a rebuild) into stable as well, though,
so that at least 4.0-stable will tolerate signatures. (Distros
backporting this would be a good idea as well.)

Christian Borntraeger May 6, 2019, 11:24 a.m. UTC | #19

On 06.05.19 13:23, Cornelia Huck wrote:
> On Mon, 6 May 2019 13:13:55 +0200
> Christian Borntraeger <[e-mail]> wrote:
> 
>> On 06.05.19 13:05, Cornelia Huck wrote:
>>> On Mon, 6 May 2019 12:46:50 +0200
>>> Christian Borntraeger <[e-mail]> wrote:
>>>   
>>>> On 06.05.19 12:34, Cornelia Huck wrote:  
>>>>> On Mon, 6 May 2019 12:18:42 +0200
>>>>> Christian Borntraeger <[e-mail]> wrote:  
>>>   
>>>>>> I think we should not. Those entries might have sematic elements that the guest
>>>>>> wants to enforce. I do not think that this will come, but imagine a boot entry
>>>>>> that mandates some security wishes (e.g. do only run on non-shared cores).    
>>>>>
>>>>> Can we split the namespace for BOOT_SCRIPT into 'ignore if you don't
>>>>> know what that is' and 'fail if you don't know what that is'? I'm
>>>>> completely confused how 'optional' those entries are supposed to be...    
>>>>
>>>> Since we do not know if and what future entries will come the current default
>>>> of failing seems the best approach. We can then add things to pc-bios when
>>>> necessary.  
>>>
>>> That's where I'm coming from: Have some values where unknown entries
>>> lead to (desired) failure, and others where unknown entries are simply
>>> ignored. That would give us automatic toleration for optional entries.  
>>
>> Well, this is the first new entry after 14 years of list-directed-ipl so there
>> is a slight chance to over-engineer here ;-)
>>
>> In the end this is a field that does not belong to Linux-only, it is also defined
>> by the machine architecture.
> 
> Yeah, I understand that having to get this into the main architecture
> makes this harder to change.
> 
> If there is nothing coming in the foreseeable future that would need
> toleration (and not failure), it's probably not worth spending more
> time on that and we should just go with this patch.
> 
> I'd recommend putting this (+ a rebuild) into stable as well, though,
> so that at least 4.0-stable will tolerate signatures. (Distros
> backporting this would be a good idea as well.)

Yes, that makes sense.

Jason J. Herne May 6, 2019, 1:03 p.m. UTC | #20

On 5/6/19 4:08 AM, Christian Borntraeger wrote:
> On 03.05.19 11:34, Thomas Huth wrote:
>> On 29/04/2019 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 <[e-mail]>
>>> Reviewed-by: Farhan Ali <[e-mail]>
>>> ---
>>>   pc-bios/s390-ccw/bootmap.c | 19 +++++++++++++++++--
>>>   pc-bios/s390-ccw/bootmap.h | 10 ++++++----
>>>   2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> Acked-by: Thomas Huth <[e-mail]>
>>
>> Jason, Christian, do you have any other changes to the s390-ccw firmware
>> currently pending that you plan to send out within the next week? If so,
>> I'll wait a little bit longer with building a new binary for the
>> repository ... if not, I'll pick this patch up next week.
> 
> 
> I have nothing at the moment.
> 

I also have nothing right now.

Patch

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;