On 5/20/25 6:25 AM, Thomas Huth wrote:
> On 09/05/2025 00.50, 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 <[email protected]>
>> ---
>> pc-bios/s390-ccw/Makefile | 3 +-
>> pc-bios/s390-ccw/bootmap.c | 192 +++++++++++++++++++++++++++++++++-
>> 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 | 175 +++++++++++++++++++++++++++++++
>> pc-bios/s390-ccw/secure-ipl.h | 109 +++++++++++++++++++
>> 9 files changed, 558 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 3dd09fda7e..06cea0929a 100644
>> --- a/pc-bios/s390-ccw/bootmap.c
>> +++ b/pc-bios/s390-ccw/bootmap.c
>> @@ -15,6 +15,7 @@
>> #include "bootmap.h"
>> #include "virtio.h"
>> #include "bswap.h"
>> +#include "secure-ipl.h"
>>
>> #ifdef DEBUG
>> /* #define DEBUG_FALLBACK */
>> @@ -34,6 +35,13 @@ 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 certificates */
>> +static uint8_t certs_sec[CERT_MAX_SIZE * MAX_CERTIFICATES];
>
> If I calculated correctly, that's a buffer of 512 kB... That's quite huge
> already. Would it be possible to malloc() it only if we really need this
> instead of statically allocating it?
>
>> +/* sector for storing signatures */
>> +static uint8_t sig_sec[MAX_SECTOR_SIZE]
>> __attribute__((__aligned__(PAGE_SIZE)));
>> +
>> +ipl_print_func_t zipl_secure_print_func;
>> +
>> /*
>> * Match two CCWs located after PSW and eight filler bytes.
>> * From libmagic and arch/s390/kernel/head.S.
>> @@ -676,6 +684,155 @@ static int zipl_load_segment(ComponentEntry *entry,
>> uint64_t address)
>> return comp_len;
>> }
>>
>> +static uint32_t zipl_handle_sig_entry(ComponentEntry *entry)
>> +{
>> + 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_index, *cert, cert_len);
>> + cert_table[cert_idx] = cert_index;
>> + *cert += cert_len;
>
> So zipl_secure_cert_list_add() checks for the index not going beyond
> MAX_CERTIFICATES, but here you ignore that error and update cert_table and
> *cert anyway? Sounds like a potential bug to me.
>
If zipl_secure_request_certificate() successfully retrieves a
certificate from the S390 certificate store, updating the corresponding
entry in cert_table should not pose any issues. This is because we
strictly limit the number of certificates to MAX_CERTIFICATES, and
cert_idx is guaranteed to be within the range [0, 63].
The size of cert_table and the memory allocated for *cert are both
defined based on MAX_CERTIFICATES and MAX_CERT_SIZE, so as long as the
request is successful, the update is safe.
The index check in zipl_secure_cert_list_add() ensures that the IIRB
cert list does not exceed the valid range defined by MAX_CERTIFICATES,
preventing out-of-bound memory overwrites.
>> + } else {
>> + puts("Could not get certificate");
>> + return -1;
>> + }
>> +
>> + /* increment cert_index for the next cert entry */
>> + return ++cert_index;
>> + }
>> +
>> + return cert_index;
>> +}
>> +
[...]
