On Wed, 2026-07-01 at 21:02 -0400, Matthew Rosato wrote: > On 7/1/26 4:48 PM, Zhuoying Cai wrote: > > Define MAX_VCENTRY_SIZE(8KB) and CERT_BUF_MAX_LEN to establish a finite > > size for a single entry VCEntry. Add validation in update_cert_store() > > to ensure certificate data does not exceed this limit. > > > > This finite size definition is needed for proper memory allocation and > > will be used in a later commit to handle VCEntry structures with known > > size constraints. > > > > Signed-off-by: Zhuoying Cai <[email protected]> > > --- > > hw/s390x/cert-store.c | 6 ++++++ > > include/hw/s390x/ipl/diag320.h | 3 +++ > > 2 files changed, 9 insertions(+) > > > > diff --git a/hw/s390x/cert-store.c b/hw/s390x/cert-store.c > > index ab3abf414b..08c38d18df 100644 > > --- a/hw/s390x/cert-store.c > > +++ b/hw/s390x/cert-store.c > > @@ -86,6 +86,12 @@ static int update_cert_store(S390IPLCertificateStore > > *cert_store, > > cert_buf_size = ROUND_UP(cert->der_size, 4); > > data_buf_size = keyid_buf_size + hash_buf_size + cert_buf_size; > > > > + if (data_buf_size > CERT_BUF_MAX_LEN) { > > + error_report("Certificate data size %zu exceeds maximum buffer > > size %ld", > > + data_buf_size, CERT_BUF_MAX_LEN); > > + return -1; > > + } > > + > > Took me a minute to realize why this needed to be its own patch vs > squashed in with hw/s390x/ipl: Create certificate store, because it > seems kind of like a fix? ... > > > if (cert_store->largest_cert_size < data_buf_size) { > > cert_store->largest_cert_size = data_buf_size; > > } > > diff --git a/include/hw/s390x/ipl/diag320.h b/include/hw/s390x/ipl/diag320.h > > index 7fda2d44fd..f3c23a3176 100644 > > --- a/include/hw/s390x/ipl/diag320.h > > +++ b/include/hw/s390x/ipl/diag320.h > > @@ -92,6 +92,9 @@ struct VCEntry { > > }; > > typedef struct VCEntry VCEntry; > > > > +#define MAX_VCENTRY_SIZE (8 * 1024) > > +#define CERT_BUF_MAX_LEN (MAX_VCENTRY_SIZE - sizeof(VCEntryHeader)) > > + > > ... it's because of VCEntryHeader. Would it be possible to instead pull > this code, the VCEntryHeader definition and CERT_NAME_MAX_LEN back > to 'hw/s390x/ipl: Create certificate store' so that we can detect > this condition right from the beginning?
+1 > > Otherwise, the code itself seems fine so if it can't be squashed then: > > Reviewed-by: Matthew Rosato <[email protected]> But I also agree it's fine. Reviewed-by: Eric Farman <[email protected]> > > > > struct VCBlockHeader { > > uint32_t in_len; > > uint32_t reserved0;
