On 5/5/26 4:18 PM, Zhuoying Cai wrote:
[...]
+}
+
+static int get_key_type(const S390IPLCertificate *cert)
+{
+ int rc;
+ Error *err = NULL;
+
+ rc = qcrypto_x509_check_ecc_curve_p521(cert->raw, cert->size, &err);
+ if (rc == -1) {
+ error_report_err(err);
+ return -1;
+ }
+
+ return (rc == 1) ? DIAG_320_VCE_KEYTYPE_ECDSA_P521 :
+ DIAG_320_VCE_KEYTYPE_SELF_DESCRIBING;
+}
+
+static int build_vce_header(VCEntry *vce, const S390IPLCertificate *cert, int
idx)
+{
+ int key_type;
+
+ vce->vce_hdr.len = cpu_to_be32(sizeof(VCEntryHeader));
+ vce->vce_hdr.cert_idx = cpu_to_be16(idx + 1);
+ memcpy(vce->vce_hdr.name, cert->name, CERT_NAME_MAX_LEN);
+
+ key_type = get_key_type(cert);
+ if (key_type == -1) {
+ return -1;
+ }
+ vce->vce_hdr.key_type = key_type;
+
+ return 0;
+}
+
+static int build_vce_data(VCEntry *vce, const S390IPLCertificate *cert)
+{
+ uint16_t keyid_field_len;
+ uint16_t hash_field_len;
+ uint32_t cert_field_len;
+ uint32_t vce_len;
+ int rc;
+
+ rc = handle_key_id(vce, cert);
+ if (rc) {
+ return -1;
+ }
+ keyid_field_len = ROUND_UP(be16_to_cpu(vce->vce_hdr.keyid_len), 4);
Maybe return the converted length from the handle_* functions, with 0
being the
error condition?
e.g. (untested)
keyid_len = handle_key_id(vce, cert);
if (!keyid_len) {
return -1;
}
Just a nit, but I think it would improve readability.
+
+ rc = handle_hash(vce, cert, keyid_field_len);
+ if (rc) {
+ return -1;
+ }
+ hash_field_len = ROUND_UP(be16_to_cpu(vce->vce_hdr.hash_len), 4);
+
+ rc = handle_cert(vce, cert, hash_field_len);
+ if (rc || !is_cert_valid(cert)) {
+ return -1;
+ }
+ cert_field_len = ROUND_UP(be32_to_cpu(vce->vce_hdr.cert_len), 4);
+
+ vce_len = sizeof(VCEntryHeader) + keyid_field_len + hash_field_len +
cert_field_len;
+ if (vce_len > be32_to_cpu(vce->vce_hdr.len)) {
+ return -1;
+ }
We already used the maximum size when we allocated space for the VCE, so
this
check is redundant I think?
+
+ vce->vce_hdr.flags |= DIAG_320_VCE_FLAGS_VALID;
+
+ /* Update vce length to reflect the actual size used by vce */
+ vce->vce_hdr.len = cpu_to_be32(vce_len);
+
+ return 0;
+}
+
+static VCEntry *diag_320_build_vce(const S390IPLCertificate *cert, int idx)
+{
+ g_autofree VCEntry *vce = NULL;
+ uint32_t vce_max_size;
+ int rc;
+
+ /*
+ * Each field of the VCE is word-aligned.
+ * Allocate enough space for the largest possible size for this VCE.
+ * As the certificate fields (key-id, hash, data) are parsed, the
+ * VCE's length field will be updated accordingly.
+ */
+ vce_max_size = sizeof(VCEntryHeader) +
+ ROUND_UP(CERT_KEY_ID_LEN, 4) +
+ ROUND_UP(CERT_HASH_LEN, 4) +
+ ROUND_UP(cert->der_size, 4);
In Patch 10, we define MAX_ENTRY_SIZE. Is that something different than
this?
Let's use the constant here unless there is a specific reason not to.
+
+ vce = g_malloc0(vce_max_size);
+ rc = build_vce_header(vce, cert, idx);
+ if (rc) {
+ /*
+ * Error occurs - VCE does not contain a valid certificate.
+ * Bit 0 of the VCE flags is 0 and the VCE length is set.
+ */
+ vce->vce_hdr.len = cpu_to_be32(VCE_INVALID_LEN);
+ goto out;
+ }
+
+ vce->vce_hdr.len = cpu_to_be32(vce_max_size);
It is not obvious later in the code if the VCE length is a true value or a
place holder for the max size, so MAX_ENTRY_SIZE would help clarify also.
+ rc = build_vce_data(vce, cert);
+ if (rc) {
+ vce->vce_hdr.len = cpu_to_be32(VCE_INVALID_LEN);
+ }
+
+out:
+ return g_steal_pointer(&vce);
+}
Actually, I wonder if we might be able to remove the diag_320_build_vce()
function since it is basically just a wrapper for building the header
and data.
If we introduce the MAX_ENTRY_SIZE constant and allocate memory before
calling
the build functions, I think we can simplify the whole thing.
See notes below in the loop to build VCEs...
+
+static int handle_diag320_store_vc(S390CPU *cpu, uint64_t addr, uint64_t r1,
uintptr_t ra,
+ S390IPLCertificateStore *cs)
+{
+ g_autofree VCBlock *vcb = NULL;
+ size_t remaining_space;
+ uint16_t first_vc_index;
+ uint16_t last_vc_index;
+ int cs_start_index;
+ int cs_end_index;
+ uint32_t in_len;
+
+ vcb = g_new0(VCBlock, 1);
+ if (s390_cpu_virt_mem_read(cpu, addr, r1, vcb, sizeof(*vcb))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return -1;
+ }
This is only the VCB header, right? That's all the space is allocated for
anyway. I find it confusing, here is my understanding, please correct
if wrong:
We are allocating a VCB and updating the header values, but we never
populate
the VCB's buffer area because each VCE being built has it's own malloc call?
Meaning when we read/write the VCB it is actually just the header, and
the VCEs
are written independently?
If that is correct, then we never really build a full VCB right? Could
we just
use the VCBHeader struct here instead to make that clear?
+
+ in_len = be32_to_cpu(vcb->vcb_hdr.in_len);
+ first_vc_index = be16_to_cpu(vcb->vcb_hdr.first_vc_index);
+ last_vc_index = be16_to_cpu(vcb->vcb_hdr.last_vc_index);
+
+ if (in_len % TARGET_PAGE_SIZE != 0) {
+ return DIAG_320_RC_INVAL_VCB_LEN;
+ }
+
+ if (first_vc_index > last_vc_index) {
+ return DIAG_320_RC_BAD_RANGE;
+ }
+
+ vcb->vcb_hdr.out_len = sizeof(VCBlockHeader);
+
+ /*
+ * DIAG 320 subcode 2 expects to query a certificate store that
+ * maintains an index origin of 1. However, the S390IPLCertificateStore
+ * maintains an index origin of 0. Thus, the indices must be adjusted
+ * for correct access into the cert store. A couple of special cases
+ * must also be accounted for.
+ */
+
+ /* Both indices are 0; return header with no certs */
+ if (first_vc_index == 0 && last_vc_index == 0) {
+ goto out;
+ }
+
+ /* Normalize indices */
+ cs_start_index = (first_vc_index == 0) ? 0 : first_vc_index - 1;
+ cs_end_index = last_vc_index - 1;
+
+ /* Requested range is outside the cert store; return header with no certs
*/
+ if (cs_start_index >= cs->count || cs_end_index >= cs->count) {
+ goto out;
+ }
+
+ remaining_space = in_len - sizeof(VCBlockHeader);
+
+ for (int i = cs_start_index; i <= cs_end_index; i++) {
+ VCEntry *vce;
What about g_autofree VCEntry *vce = g_malloc0(MAX_ENTRY_SIZE);
Collin already mentioned using g_autofree, but in addition to that if we can
allocate the memory up front that would allow us to call the build
functions directly from here e.g. (untested)
if (build_vce_header(vce, cert, i) || build_vce_data(vce, cert) {
vce->vce_hdr.len = cpu_to_be32(VCE_INVALID_LEN);
}
That would avoid the frees and steal pointer too.
+ const S390IPLCertificate *cert = &cs->certs[i];
+
+ /*
+ * Bit 0 of the VCE flags indicates whether the certificate is valid.
+ * The caller of DIAG320 subcode 2 is responsible for verifying that
+ * the VCE contains a valid certificate.
+ */
+ vce = diag_320_build_vce(cert, i);
+
+ /*
+ * If there is no more space to store the cert,
+ * set the remaining verification cert count and
+ * break early.
+ */
+ if (remaining_space < vce->vce_hdr.len) {
+ vcb->vcb_hdr.remain_ct = cpu_to_be16(last_vc_index - i);
+ g_free(vce);
+ break;
+ }
+
+ /* Write VCE */
+ if (s390_cpu_virt_mem_write(cpu, addr + vcb->vcb_hdr.out_len, r1,
+ vce, vce->vce_hdr.len)) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ g_free(vce);
+ return -1;
+ }
+
+ vcb->vcb_hdr.out_len += vce->vce_hdr.len;
+ remaining_space -= vce->vce_hdr.len;
+ vcb->vcb_hdr.stored_ct++;
+
+ g_free(vce);
+ }
+ vcb->vcb_hdr.stored_ct = cpu_to_be16(vcb->vcb_hdr.stored_ct);
+
+out:
+ vcb->vcb_hdr.out_len = cpu_to_be32(vcb->vcb_hdr.out_len);
+
+ if (s390_cpu_virt_mem_write(cpu, addr, r1, vcb, sizeof(VCBlockHeader))) {
+ s390_cpu_virt_mem_handle_exc(cpu, ra);
+ return -1;
+ }
+
+ return DIAG_320_RC_OK;
+}
+
QEMU_BUILD_BUG_MSG(sizeof(VCStorageSizeBlock) != VCSSB_MIN_LEN,
"size of VCStorageSizeBlock is wrong");
+QEMU_BUILD_BUG_MSG(sizeof(VCBlock) != 64, "size of VCBlock is wrong");
+QEMU_BUILD_BUG_MSG(sizeof(VCEntry) != 128, "size of VCEntry is wrong");
void handle_diag_320(CPUS390XState *env, uint64_t r1, uint64_t r3, uintptr_t ra)
{
@@ -270,7 +594,8 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1,
uint64_t r3, uintptr_t ra)
* for now.
*/
uint32_t ism_word0 = cpu_to_be32(DIAG_320_ISM_QUERY_SUBCODES |
- DIAG_320_ISM_QUERY_VCSI);
+ DIAG_320_ISM_QUERY_VCSI |
+ DIAG_320_ISM_STORE_VC);
if (s390_cpu_virt_mem_write(cpu, addr, r1, &ism_word0, sizeof(ism_word0))) {
s390_cpu_virt_mem_handle_exc(cpu, ra);
@@ -296,6 +621,18 @@ void handle_diag_320(CPUS390XState *env, uint64_t r1,
uint64_t r3, uintptr_t ra)
}
env->regs[r1 + 1] = rc;
break;
+ case DIAG_320_SUBC_STORE_VC:
+ if (addr & ~TARGET_PAGE_MASK) {
+ s390_program_interrupt(env, PGM_SPECIFICATION, ra);
+ return;
+ }
+
+ rc = handle_diag320_store_vc(cpu, addr, r1, ra, cs);
+ if (rc == -1) {
+ return;
+ }
+ env->regs[r1 + 1] = rc;
+ break;
default:
env->regs[r1 + 1] = DIAG_320_RC_NOT_SUPPORTED;
break;
Regards,
Jared Rossi
