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

[Christian Borntraeger]

On 06.05.19 12:34, Cornelia Huck wrote:
> On Mon, 6 May 2019 12:18:42 +0200
> Christian Borntraeger <address@hidden> 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 <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.
>>>>>  
>>>>
>>>>
>>>> ... 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.