[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL v2 18/24] ACPI ERST: support for ACPI ERST feature
From: |
Ani Sinha |
Subject: |
Re: [PULL v2 18/24] ACPI ERST: support for ACPI ERST feature |
Date: |
Fri, 13 May 2022 13:04:10 +0530 |
On Thu, May 12, 2022 at 9:59 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 6 Feb 2022 at 09:38, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > From: Eric DeVolder <eric.devolder@oracle.com>
> >
> > 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 <eric.devolder@oracle.com>
> > Reviewed-by: Ani Sinha <ani@anisinha.ca>
> > Message-Id: <1643402289-22216-6-git-send-email-eric.devolder@oracle.com>
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
>
> 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.