| Message ID | 1556543381-12671-1-git-send-email-jjherne@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390-bios: Skip bootmap signature entries | expand |
On Mon, 29 Apr 2019 09:09:41 -0400 "Jason J. Herne" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> > Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > --- > 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.
On 29.04.19 15:40, Cornelia Huck wrote: > On Mon, 29 Apr 2019 09:09:41 -0400 > "Jason J. Herne" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> >> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >> --- >> 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.
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" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> >>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >>> --- >>> 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
On Tue, 30 Apr 2019 11:24:27 +0200 Peter Oberparleiter <oberpar@linux.ibm.com> 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" <jjherne@linux.ibm.com> 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 <jjherne@linux.ibm.com> > >>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > >>> --- > >>> 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!
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 <jjherne@linux.ibm.com> > Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > --- > 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 <thuth@redhat.com> 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
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 <jjherne@linux.ibm.com> >> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >> --- >> 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 <thuth@redhat.com> > > 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.
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 <jjherne@linux.ibm.com> > Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > --- > 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.
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 <jjherne@linux.ibm.com> >> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >> --- >> 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.
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 <jjherne@linux.ibm.com> >> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >> --- >> 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.
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 <jjherne@linux.ibm.com> >>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >>> --- >>> 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
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 <jjherne@linux.ibm.com> >>>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >>>> --- >>>> 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).
On Mon, 6 May 2019 12:14:10 +0200 Christian Borntraeger <borntraeger@de.ibm.com> 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 <jjherne@linux.ibm.com> > >> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > >> --- > >> 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?
On Mon, 6 May 2019 12:18:42 +0200 Christian Borntraeger <borntraeger@de.ibm.com> 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 <jjherne@linux.ibm.com> > >>>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> > >>>> --- > >>>> 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...
On 06.05.19 12:30, Cornelia Huck wrote: > On Mon, 6 May 2019 12:14:10 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> 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 <jjherne@linux.ibm.com> >>>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >>>> --- >>>> 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?
On 06.05.19 12:34, Cornelia Huck wrote: > On Mon, 6 May 2019 12:18:42 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> 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 <jjherne@linux.ibm.com> >>>>>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >>>>>> --- >>>>>> 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.
On Mon, 6 May 2019 12:46:50 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06.05.19 12:34, Cornelia Huck wrote: > > On Mon, 6 May 2019 12:18:42 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> 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.
On 06.05.19 13:05, Cornelia Huck wrote: > On Mon, 6 May 2019 12:46:50 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 06.05.19 12:34, Cornelia Huck wrote: >>> On Mon, 6 May 2019 12:18:42 +0200 >>> Christian Borntraeger <borntraeger@de.ibm.com> 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.
On Mon, 6 May 2019 13:13:55 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 06.05.19 13:05, Cornelia Huck wrote: > > On Mon, 6 May 2019 12:46:50 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> On 06.05.19 12:34, Cornelia Huck wrote: > >>> On Mon, 6 May 2019 12:18:42 +0200 > >>> Christian Borntraeger <borntraeger@de.ibm.com> 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.)
On 06.05.19 13:23, Cornelia Huck wrote: > On Mon, 6 May 2019 13:13:55 +0200 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 06.05.19 13:05, Cornelia Huck wrote: >>> On Mon, 6 May 2019 12:46:50 +0200 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>> >>>> On 06.05.19 12:34, Cornelia Huck wrote: >>>>> On Mon, 6 May 2019 12:18:42 +0200 >>>>> Christian Borntraeger <borntraeger@de.ibm.com> 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.
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 <jjherne@linux.ibm.com> >>> Reviewed-by: Farhan Ali <alifm@linux.ibm.com> >>> --- >>> 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 <thuth@redhat.com> >> >> 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.
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;