On Thu, May 12, 2022 at 9:59 PM Peter Maydell <[email protected]> wrote: > > On Sun, 6 Feb 2022 at 09:38, Michael S. Tsirkin <[email protected]> wrote: > > > > From: Eric DeVolder <[email protected]> > > > > This implements a PCI device for ACPI ERST. This implements the > > non-NVRAM "mode" of operation for ERST as it is supported by > > Linux and Windows. > > > > Signed-off-by: Eric DeVolder <[email protected]> > > Reviewed-by: Ani Sinha <[email protected]> > > Message-Id: <[email protected]> > > Reviewed-by: Michael S. Tsirkin <[email protected]> > > Signed-off-by: Michael S. Tsirkin <[email protected]> > > --- > > Hi; Coverity points out a bug in this function (CID 1487125): > > > +static void check_erst_backend_storage(ERSTDeviceState *s, Error **errp) > > +{ > > + ERSTStorageHeader *header; > > + uint32_t record_size; > > + > > + header = memory_region_get_ram_ptr(s->hostmem_mr); > > + s->header = header; > > + > > + /* Ensure pointer to header is 64-bit aligned */ > > + g_assert(QEMU_PTR_IS_ALIGNED(header, sizeof(uint64_t))); > > + > > + /* > > + * Check if header is uninitialized; HostMemoryBackend inits to 0 > > + */ > > + if (le64_to_cpu(header->magic) == 0UL) { > > + make_erst_storage_header(s); > > + } > > + > > + /* Validity check record_size */ > > + record_size = le32_to_cpu(header->record_size); > > + if (!( > > + (record_size) && /* non zero */ > > + (record_size >= UEFI_CPER_RECORD_MIN_SIZE) && > > + (((record_size - 1) & record_size) == 0) && /* is power of 2 */ > > + (record_size >= 4096) /* PAGE_SIZE */ > > + )) { > > + error_setg(errp, "ERST record_size %u is invalid", record_size); > > Here we check that record_size is sensible, including that it is > not zero. But we forget to return early after error_setg(), which means... > > > + } > > + > > + /* Validity check header */ > > + if (!( > > + (le64_to_cpu(header->magic) == ERST_STORE_MAGIC) && > > + ((le32_to_cpu(header->storage_offset) % record_size) == 0) && > > + (le16_to_cpu(header->version) == 0x0100) && > > + (le16_to_cpu(header->reserved) == 0) > > + )) { > > + error_setg(errp, "ERST backend storage header is invalid"); > > + } > > + > > + /* Check storage_size against record_size */ > > + if (((s->storage_size % record_size) != 0) || > > ...that we fall through to here, where we will divide by zero if > record_size is 0. > > > + (record_size > s->storage_size)) { > > + error_setg(errp, "ACPI ERST requires storage size be multiple of " > > + "record size (%uKiB)", record_size); > > + } > > + > > + /* Compute offset of first and last record storage slot */ > > + s->first_record_index = le32_to_cpu(header->storage_offset) > > + / record_size; > > + s->last_record_index = (s->storage_size / record_size); > > +} > > + > > The fix is probably simply to add return statements after each error_setg() > in the function.
Ah yes. OK I will send a patch as soon as I am able.
