On 08/04/2025 17.55, Zhuoying Cai wrote:
Create a certificate store for boot certificates used for secure IPL.
Load certificates from the -boot-certificate option into the cert store.
Currently, only x509 certificates in DER format and uses SHA-256 hashing
algorithm are supported, as these are the types required for secure boot
on s390.
Signed-off-by: Zhuoying Cai <zy...@linux.ibm.com>
---
...
+static size_t cert2buf(char *path, size_t max_size, char **cert_buf)
+{
+ size_t size;
+ g_autofree char *buf;
+ buf = g_malloc(max_size);
+
+ if (!g_file_get_contents(path, &buf, &size, NULL) ||
+ size == 0 || size > max_size) {
+ return 0;
+ }
+
+ *cert_buf = g_steal_pointer(&buf);
+
+ return size;
+}
This function looks quite wrong to me. Why is there a g_malloc() in here if
g_file_get_contents() already allocates the memory?
And why do we need a max_size here? If there is a reason, please add a
proper comment in the source code.
+#ifdef CONFIG_GNUTLS
+int g_init_cert(uint8_t *raw_cert, size_t cert_size, gnutls_x509_crt_t *g_cert)
Please don't use a "g_" prefix here - otherwise that way the function could
be confused with the functions from the glib.
+{
+ int rc;
+
+ if (gnutls_x509_crt_init(g_cert) < 0) {
+ return -1;
+ }
+
+ gnutls_datum_t datum_cert = {raw_cert, cert_size};
+ rc = gnutls_x509_crt_import(*g_cert, &datum_cert, GNUTLS_X509_FMT_DER);
+ if (rc) {
+ gnutls_x509_crt_deinit(*g_cert);
+ return rc;
+ }
+
+ return 0;
+}
+#endif /* CONFIG_GNUTLS */
+
+static int init_cert_x509_der(size_t size, char *raw, S390IPLCertificate
**qcert)
I'd maybe rather use "S390IPLCertificate *" as return type instead of "int"
and return a NULL in case of errors.
+{
+#ifdef CONFIG_GNUTLS
+ gnutls_x509_crt_t g_cert = NULL;
+ g_autofree S390IPLCertificate *q_cert;
+ size_t key_id_size;
+ size_t hash_size;
+ int rc;
+
+ rc = g_init_cert((uint8_t *)raw, size, &g_cert);
+ if (rc) {
+ if (rc == GNUTLS_E_ASN1_TAG_ERROR) {
+ error_report("The certificate is not in DER format");
+ }
+ return -1;
+ }
+
+ rc = gnutls_x509_crt_get_key_id(g_cert, GNUTLS_KEYID_USE_SHA256, NULL,
&key_id_size);
Is that documented somewhere that you can call gnutls_x509_crt_get_key_id()
like this? The docs that I found about this function do not say anything
about passing NULL here, they rather recommend to use a buffer of size 20 by
default?
+ if (rc != GNUTLS_E_SHORT_MEMORY_BUFFER) {
+ error_report("Failed to get certificate key ID size");
+ goto out;
+ }
+
+ rc = gnutls_x509_crt_get_fingerprint(g_cert, GNUTLS_DIG_SHA256, NULL,
&hash_size);
For this function, the NULL pointer handling is documented, so here it seems
to be OK.
+ if (rc != GNUTLS_E_SHORT_MEMORY_BUFFER) {
+ error_report("Failed to get certificate hash size");
+ goto out;
+ }
+
+ q_cert = g_malloc(sizeof(*q_cert));
Please use g_new() for allocating memory for structures instead.
+ q_cert->size = size;
+ q_cert->key_id_size = key_id_size;
+ q_cert->hash_size = hash_size;
+ q_cert->raw = raw;
+ q_cert->format = GNUTLS_X509_FMT_DER;
+ *qcert = g_steal_pointer(&q_cert);
If there is no "return" between the allocation and the final "return 0", you
can also drop the g_autofree and g_steal_pointer from this function.
+ gnutls_x509_crt_deinit(g_cert);
+
+ return 0;
+out:
+ gnutls_x509_crt_deinit(g_cert);
+ return -1;
+#else
+ error_report("Cryptographic library is not enabled")
+ return -1;
+#endif /* #define CONFIG_GNUTLS */
+}
+
+static int check_path_type(const char *path)
+{
+ struct stat path_stat;
+
+ stat(path, &path_stat);
+
+ if (S_ISDIR(path_stat.st_mode)) {
+ return S_IFDIR;
+ } else if (S_ISREG(path_stat.st_mode)) {
+ return S_IFREG;
+ } else {
+ return -1;
+ }
+}
+
+static int init_cert(char *paths, S390IPLCertificate **qcert)
as with previous function, use "S390IPLCertificate *" as return type instead
of "int" ?
+{
+ char *buf;
+ char vc_name[VC_NAME_LEN_BYTES];
+ const gchar *filename;
+ size_t size;
+
+ filename = g_path_get_basename(paths);
g_path_get_basename() returns an allocated string. You've finally got to
free it again to avoid leaking memory. I'd suggest declaring filename with
g_autofree.
+ size = cert2buf(paths, CERT_MAX_SIZE, &buf);
+ if (size == 0) {
+ error_report("Failed to load certificate: %s", paths);
+ return -1;
+ }
+
+ if (init_cert_x509_der(size, buf, qcert) < 0) {
+ error_report("Failed to initialize certificate: %s", paths);
+ return -1;
+ }
+
+ /*
+ * Left justified certificate name with padding on the right with blanks.
+ * Convert certificate name to EBCDIC.
+ */
+ strpadcpy(vc_name, VC_NAME_LEN_BYTES, filename, ' ');
+ ebcdic_put((*qcert)->vc_name, vc_name, VC_NAME_LEN_BYTES);
+
+ return 0;
+}
+
+static void update_cert_store(S390IPLCertificateStore *cert_store,
+ S390IPLCertificate *qcert)
+{
+ size_t data_size;
+
+ data_size = qcert->size + qcert->key_id_size + qcert->hash_size;
+
+ if (cert_store->max_cert_size < data_size) {
+ cert_store->max_cert_size = data_size;
+ }
+
+ cert_store->certs[cert_store->count] = *qcert;
+ cert_store->total_bytes += data_size;
+ cert_store->count++;
+}
+
+static GPtrArray *get_cert_paths(void)
+{
+ const char *path;
+ gchar **paths;
+ int path_type;
+ GDir *dir = NULL;
+ const gchar *filename;
+ GPtrArray *cert_path_builder;
+
+ cert_path_builder = g_ptr_array_new();
+
+ path = s390_get_boot_certificates();
+ if (path == NULL) {
+ return cert_path_builder;
+ }
+
+ paths = g_strsplit(path, ":", -1);
Free the memory that has been allocated by the g_strsplit at the end of the
function?
+ while (*paths) {
+ /* skip empty certificate path */
+ if (!strcmp(*paths, "")) {
+ paths += 1;
+ continue;
+ }
+
+ path_type = check_path_type(*paths);
+ if (path_type == S_IFREG) {
+ g_ptr_array_add(cert_path_builder, (gpointer) *paths);
... that should likely g_strdup(*paths) when we want to free paths at the end.
+ } else if (path_type == S_IFDIR) {
+ dir = g_dir_open(*paths, 0, NULL);
+
+ while ((filename = g_dir_read_name(dir))) {
+ gchar *cert_path = NULL;
+ cert_path = g_build_filename(*paths, filename, NULL);
+ g_ptr_array_add(cert_path_builder, (gpointer) cert_path);
+ }
+
+ g_dir_close(dir);
+ }
+
+ paths += 1;
+ }
+
+ return cert_path_builder;
+}
+
+void s390_ipl_create_cert_store(S390IPLCertificateStore *cert_store)
+{
+ GPtrArray *cert_path_builder;
+
+ cert_path_builder = get_cert_paths();
+ if (cert_path_builder->len == 0) {
+ g_ptr_array_free(cert_path_builder, true);
+ return;
+ }
+
+ cert_store->max_cert_size = 0;
+ cert_store->total_bytes = 0;
+
+ for (int i = 0; i < cert_path_builder->len; i++) {
+ S390IPLCertificate *qcert = NULL;
+ if (init_cert((char *) cert_path_builder->pdata[i], &qcert) < 0) {
+ continue;
Maybe invert the logic to call update_cert_store() in case of success, than
you don't need the "continue" anymore?
+ }
+
+ update_cert_store(cert_store, qcert);
+ }
+
+ g_ptr_array_free(cert_path_builder, true);
+}
Thomas
