Re: [PATCH v4 20/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/21/25 17:50, Zhuoying Cai wrote:
> On 7/11/25 6:50 PM, Collin Walling wrote:
>> On 7/11/25 5:10 PM, Zhuoying Cai wrote:
[...]
>>>
>>> +typedef enum ZiplBootMode {
>>> +ZIPL_NORMAL_MODE = 1,
>>> +ZIPL_SECURE_AUDIT_MODE = 2,
>>> +} ZiplBootMode;
>>
>> These should be ZIPL_BOOT_MODE_*
>>
>> Also, is there a reason why the list starts at 1 and not defaulting to
>> the implicit 0?
>>
>
> boot_mode is a global variable defined in pc-bios/s390-ccw/main.c, and
> it defaults to 0, which indicates that the boot mode hasn’t been
> determined yet.
>
> We start the list at 1 to reserve 0 as the implicit “undefined” or “not
> yet set” value. The actual boot mode is only set later when boot_mode == 0:
> if (boot_mode == 0) {
> boot_mode = zipl_mode(iplb->hdr_flags);
> }
> This allows us to distinguish between an unset state and valid boot modes.
>
I would have thought to default the boot mode to NORMAL, but I haven't
had my eyes on the BIOS patches in a bit. Unsure what makes sense.
For now, I'd suggest adding ZIPL_MODE_UNSPECIFIED (or something like
that) to the list.
[...]
--
Regards,
Collin
Re: [PATCH v4 20/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/11/25 6:50 PM, Collin Walling wrote:
> On 7/11/25 5:10 PM, Zhuoying Cai wrote:
>> Enable secure IPL in audit mode, which performs signature verification,
>> but any error does not terminate the boot process. Only warnings will be
>> logged to the console instead.
>>
>> Add a comp_len variable to store the length of a segment in
>> zipl_load_segment. comp_len variable is necessary to store the
>> calculated segment length and is used during signature verification.
>> Return the length on success, or a negative return code on failure.
>>
>> Secure IPL in audit mode requires at least one certificate provided in
>> the key store along with necessary facilities (Secure IPL Facility,
>> Certificate Store Facility and secure IPL extension support).
>>
>> Note: Secure IPL in audit mode is implemented for the SCSI scheme of
>> virtio-blk/virtio-scsi devices.
>>
>> Signed-off-by: Zhuoying Cai
[snip...]
>> +static int zipl_run_secure(ComponentEntry *entry, uint8_t *tmp_sec)
>> +{
>> +IplDeviceComponentList comps;
>> +IplSignatureCertificateList certs;
>> +uint64_t *cert = NULL;
>> +int cert_index = 0;
>> +int comp_index = 0;
>> +uint64_t comp_addr;
>> +int comp_len;
>> +bool have_sig;
>> +uint32_t sig_len;
>> +uint64_t cert_len = -1;
>> +uint8_t cert_idx = -1;
>> +bool verified;
>> +uint32_t certs_len;
>> +/*
>> + * Store indices of cert entry that have already used for signature
>> verification
>> + * to prevent allocating the same certificate multiple times.
>> + * cert_table index: index of certificate from qemu cert store used for
>> verification
>> + * cert_table value: index of cert entry in cert list that contains the
>> certificate
>> + */
>> +int cert_table[MAX_CERTIFICATES] = { [0 ... MAX_CERTIFICATES - 1] = -1};
>> +int signed_count = 0;
>> +
>> +if (!zipl_secure_ipl_supported()) {
>> +return -1;
>> +}
>> +
>> +zipl_secure_init_lists(&comps, &certs);
>> +certs_len = zipl_secure_get_certs_length();
>> +cert = malloc(certs_len);
>> +
>> +have_sig = false;
>
> You can get rid of this boolean and simply use sig_len as the indicator
> that there were nonzero bytes read when a signature entry was detected
> in the previous loop. Where you set have_sig to false below, you can
> set sig_len to 0 to reset it.
>
>> +while (entry->component_type == ZIPL_COMP_ENTRY_LOAD ||
>> + entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
>> +
>
> I'll be honest, I'm not a big fan of neither the design of this while
> loop nor the one in zipl_run_normal. I'd prefer something like:
>
> while (entry->component_type != ZIPL_COMP_ENTRY_EXEC) {
>
> // sanity checking
>
> switch (entry->component_type) {
> case ZIPL_COMP_ENTRY_SIGNATURE:
> ...
> break;
> case ZIPL_COMP_ENTRY_LOAD:
> ...
> break;
> default:
> // Unrecognized entry type, return error
> }
>
> entry++;
> }
>
> The above makes it clear that we're loading in segments until the exec
> entry is found, and the handling for each recognized component type is
> clearly labeled and organized.
>
> I won't speak for making this change to the zipl_run_normal function
> yet, as it may introduce too many changes in this patch series.
>
>> +if (entry->component_type == ZIPL_COMP_ENTRY_SIGNATURE) {
>> +/* There should never be two signatures in a row */
>> +if (have_sig) {
>> +return -1;
>> +}
>> +
>> +sig_len = zipl_handle_sig_entry(entry);
>> +if (sig_len < 0) {
>> +return -1;
>> +}
>> +
>> +have_sig = true;
>> +} else {
>> +comp_addr = entry->compdat.load_addr;
>> +comp_len = zipl_load_segment(entry, comp_addr);
>> +if (comp_len < 0) {
>> +return -1;
>> +}
>> +
>> +if (have_sig) {
>
> If you use my idea to re-write this loop, you'll be able to reduce one
> level of indentation of the code that follows by checking the inverse
> condition:
>
> if (!have_sig) { // or if (!sig_len)
> break;
>
>> +verified = verify_signature(comp_len, comp_addr,
>> +sig_len, (uint64_t)sig_sec,
>> +&cert_len, &cert_idx);
>> +
>> +if (verified) {
>> +cert_index = handle_certificate(cert_table, &cert,
>> +cert_len, cert_idx,
>> +&certs, cert_index);
>> +if (cert_index == -1) {
>> +return -1;
>> +}
>> +
>> +puts("Verified component");
>> +zipl_secure_comp_list_add(&comps, comp_index,
>> cert_table[cert_idx],
>> +
Re: [PATCH v4 20/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/14/25 11:34 AM, Thomas Huth wrote:
> On 14/07/2025 16.54, Jared Rossi wrote:
>>
>> [snip...]
+
+ entry++;
+
+ if ((uint8_t *)(&entry[1]) > tmp_sec + MAX_SECTOR_SIZE) {
+ puts("Wrong entry value");
+ return -EINVAL;
+ }
>>> Can someone who is more informed than I am of the IPL process please
>>> explain to me what is the purpose of the above check? Why does it check
>>> if the next entry, the one which isn't going to be inspected/loaded, is
>>> within the bounds of tmp_sec? This has been here since this file's
>>> inception and I can't find any documentation or mention that supports it.
>>>
>>> This code precludes any of the secure IPL changes.
>>>
>>> Was this actually meant to be entry[0] to ensure the actual entry we
>>> want to work on is not outside the bounds of tmp_sec? Or perhaps it was
>>> meant to be done before the increment to entry?
>>>
>>
>> I noticed that as well and came to the same conclusions as you, which is to
>> say,
>> it has always been that way and it is not clear what the purpose is, but it
>> does
>> not appear to have any impact on the proposed secure IPL functionality.
Fair enough. Let's keep the current code in and address it later.
Thanks, Jared.
>
> I think it's meant as a check for the *end* of entry[0], so it's likely just
> a quirky way of saying:
>
> if (((uint8_t *)entry) + sizeof(*entry) > tmp_sec + MAX_SECTOR_SIZE)
>
> ?
>
> Thomas
>
This makes a lot more sense to me. Thanks, Thomas.
--
Regards,
Collin
Re: [PATCH v4 20/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 14/07/2025 16.54, Jared Rossi wrote:
[snip...]
+
+ entry++;
+
+ if ((uint8_t *)(&entry[1]) > tmp_sec + MAX_SECTOR_SIZE) {
+ puts("Wrong entry value");
+ return -EINVAL;
+ }
Can someone who is more informed than I am of the IPL process please
explain to me what is the purpose of the above check? Why does it check
if the next entry, the one which isn't going to be inspected/loaded, is
within the bounds of tmp_sec? This has been here since this file's
inception and I can't find any documentation or mention that supports it.
This code precludes any of the secure IPL changes.
Was this actually meant to be entry[0] to ensure the actual entry we
want to work on is not outside the bounds of tmp_sec? Or perhaps it was
meant to be done before the increment to entry?
I noticed that as well and came to the same conclusions as you, which is to
say,
it has always been that way and it is not clear what the purpose is, but it
does
not appear to have any impact on the proposed secure IPL functionality.
I think it's meant as a check for the *end* of entry[0], so it's likely just
a quirky way of saying:
if (((uint8_t *)entry) + sizeof(*entry) > tmp_sec + MAX_SECTOR_SIZE)
?
Thomas
Re: [PATCH v4 20/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
[snip...]
+
+entry++;
+
+if ((uint8_t *)(&entry[1]) > tmp_sec + MAX_SECTOR_SIZE) {
+puts("Wrong entry value");
+return -EINVAL;
+}
Can someone who is more informed than I am of the IPL process please
explain to me what is the purpose of the above check? Why does it check
if the next entry, the one which isn't going to be inspected/loaded, is
within the bounds of tmp_sec? This has been here since this file's
inception and I can't find any documentation or mention that supports it.
This code precludes any of the secure IPL changes.
Was this actually meant to be entry[0] to ensure the actual entry we
want to work on is not outside the bounds of tmp_sec? Or perhaps it was
meant to be done before the increment to entry?
I noticed that as well and came to the same conclusions as you, which is
to say,
it has always been that way and it is not clear what the purpose is, but
it does
not appear to have any impact on the proposed secure IPL functionality.
I agree
that it seems somehow strange, but I believe any changes/fixes would be
outside
the scope of this item.
In my opinion, since this is already a large patch series, it is better
to leave
it alone for now unless we find a compelling reason to change it
immediately.
Regards,
Jared Rossi
Re: [PATCH v4 20/28] pc-bios/s390-ccw: Add signature verification for secure IPL in audit mode
On 7/11/25 5:10 PM, Zhuoying Cai wrote:
> Enable secure IPL in audit mode, which performs signature verification,
> but any error does not terminate the boot process. Only warnings will be
> logged to the console instead.
>
> Add a comp_len variable to store the length of a segment in
> zipl_load_segment. comp_len variable is necessary to store the
> calculated segment length and is used during signature verification.
> Return the length on success, or a negative return code on failure.
>
> Secure IPL in audit mode requires at least one certificate provided in
> the key store along with necessary facilities (Secure IPL Facility,
> Certificate Store Facility and secure IPL extension support).
>
> Note: Secure IPL in audit mode is implemented for the SCSI scheme of
> virtio-blk/virtio-scsi devices.
>
> Signed-off-by: Zhuoying Cai
I would suggest encapsulating all secure-boot related functions into the
new secure-ipl.c|h files to help keep bootmap.c less bloated. I would
suggest moving the following to the new file:
- zipl_handle_sig_entry
- handle_certificate
- zipl_run_secure (this should probably be the only non-static func,
since it'll be used by bootmap.c, and you'll probably need to mess with
the levels of indirection for the params)
Of course, make all functions used by zipl_run_secure as static since
they'll only be used there.
I'll revist this patch later to verify the SCLP bits and triple-check
the data structure layouts.
More comments below:
> ---
> pc-bios/s390-ccw/Makefile | 3 +-
> pc-bios/s390-ccw/bootmap.c| 193 +-
> pc-bios/s390-ccw/bootmap.h| 9 ++
> pc-bios/s390-ccw/main.c | 9 ++
> pc-bios/s390-ccw/s390-ccw.h | 14 +++
> pc-bios/s390-ccw/sclp.c | 44
> pc-bios/s390-ccw/sclp.h | 6 ++
> pc-bios/s390-ccw/secure-ipl.c | 190 +
> pc-bios/s390-ccw/secure-ipl.h | 98 +
> 9 files changed, 563 insertions(+), 3 deletions(-)
> create mode 100644 pc-bios/s390-ccw/secure-ipl.c
> create mode 100644 pc-bios/s390-ccw/secure-ipl.h
>
> diff --git a/pc-bios/s390-ccw/Makefile b/pc-bios/s390-ccw/Makefile
> index dc69dd484f..fedb89a387 100644
> --- a/pc-bios/s390-ccw/Makefile
> +++ b/pc-bios/s390-ccw/Makefile
> @@ -34,7 +34,8 @@ QEMU_DGFLAGS = -MMD -MP -MT $@ -MF $(@D)/$(*F).d
> .PHONY : all clean build-all distclean
>
> OBJECTS = start.o main.o bootmap.o jump2ipl.o sclp.o menu.o netmain.o \
> - virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o
> + virtio.o virtio-net.o virtio-scsi.o virtio-blkdev.o cio.o dasd-ipl.o \
> + secure-ipl.o
>
> SLOF_DIR := $(SRC_PATH)/../../roms/SLOF
>
> diff --git a/pc-bios/s390-ccw/bootmap.c b/pc-bios/s390-ccw/bootmap.c
> index 2513e6c131..0827459803 100644
> --- a/pc-bios/s390-ccw/bootmap.c
> +++ b/pc-bios/s390-ccw/bootmap.c
> @@ -8,6 +8,7 @@
> * directory.
> */
>
> +#include
> #include
> #include
> #include "s390-ccw.h"
> @@ -15,6 +16,7 @@
> #include "bootmap.h"
> #include "virtio.h"
> #include "bswap.h"
> +#include "secure-ipl.h"
>
> #ifdef DEBUG
> /* #define DEBUG_FALLBACK */
> @@ -34,6 +36,9 @@ static uint8_t sec[MAX_SECTOR_SIZE*4]
> __attribute__((__aligned__(PAGE_SIZE)));
> const uint8_t el_torito_magic[] = "EL TORITO SPECIFICATION"
>"\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0";
>
> +/* sector for storing signatures */
> +static uint8_t sig_sec[MAX_SECTOR_SIZE]
> __attribute__((__aligned__(PAGE_SIZE)));
Since only zipl_secure_run is making use of this scratch space, I'd
suggest declaring some chunk of mem within that function and then
passing it as a parameter to zipl_handle_sig_entry. It'll also make it
a lot easier to understand what is going on in the
ZIPL_COMP_ENTRY_SIGNATURE case.
> +
> /*
> * Match two CCWs located after PSW and eight filler bytes.
> * From libmagic and arch/s390/kernel/head.S.
> @@ -676,6 +681,159 @@ static int zipl_load_segment(ComponentEntry *entry,
> uint64_t address)
> return comp_len;
> }
>
> +static uint32_t zipl_handle_sig_entry(ComponentEntry *entry)
I think "zipl_load_signature" reads better.
> +{
> +uint32_t sig_len;
> +
> +if (zipl_load_segment(entry, (uint64_t)sig_sec) < 0) {
> +return -1;
> +}
> +
> +if (entry->compdat.sig_info.format != DER_SIGNATURE_FORMAT) {
> +puts("Signature is not in DER format");
> +return -1;
> +}
> +sig_len = entry->compdat.sig_info.sig_len;
> +
> +return sig_len;
> +}
> +
> +static int handle_certificate(int *cert_table, uint64_t **cert,
> + uint64_t cert_len, uint8_t cert_idx,
> + IplSignatureCertificateList *certs, int
> cert_index)
> +{
> +bool unused;
> +
> +unused = cert_table[cert_idx] == -1;
> +if (unused) {
> +if (zipl_secure_request_certificate(*cert, cert_idx)) {
> +zipl_secure_cert_list_add(certs, cert_ind
