Re: [PATCH] hw/acpi/erst.c: Fix memset argument order

[Markus Armbruster]

"Christian A. Ehrhardt" <lk@c--e.de> writes:

> Fix memset argument order: The second argument is
> the value, the length goes last.

Impact of the bug?

> Cc: Eric DeVolder <eric.devolder@oracle.com>
> Cc: qemu-stable@nongnu.org
> Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
>  hw/acpi/erst.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index df856b2669..26391f93ca 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -716,7 +716,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
       exchange_length = memory_region_size(&s->exchange_mr);

This is the size of the exchange buffer.

Aside: it's unsigned int, but memory_region_size() returns uint64_t.
Unclean if it fits, bug if it doesn't.

       /* Validate record_offset */
       if (s->record_offset > (exchange_length - UEFI_CPER_RECORD_MIN_SIZE)) {
           return STATUS_FAILED;
       }

       /* Obtain pointer to record in the exchange buffer */
       exchange = memory_region_get_ram_ptr(&s->exchange_mr);
       exchange += s->record_offset;

       /* Validate CPER record_length */
       memcpy((uint8_t *)&record_length, 
&exchange[UEFI_CPER_RECORD_LENGTH_OFFSET],
           sizeof(uint32_t));

Aside: record_length = *(uint32_t *)exchange[UEFI_CPER_RECORD_LENGTH_OFFSET]
would do, since UEFI_CPER_RECORD_LENGTH_OFFSET is a multiple of 4.

       record_length = le32_to_cpu(record_length);
       if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
           return STATUS_FAILED;
       }
       if ((s->record_offset + record_length) > exchange_length) {
           return STATUS_FAILED;
       }

This ensures there are at least @record_length bytes of space left in
the exchange buffer.  Good.

       [...]
>      if (nvram) {
>          /* Write the record into the slot */
>          memcpy(nvram, exchange, record_length);

This first copies @record_length bytes into the exchange buffer.

> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);

The new code pads it to the full exchange buffer size.

The old code writes 0xFF bytes.

If 0xFF < exchange_length - record_length, the padding doesn't extend to
the end of the buffer.  Impact?

If 0xFF > exchange_length - record_length, we write beyond the end of
the buffer.  Impact?

>          /* If a new record, increment the record_count */
>          if (!record_found) {
>              uint32_t record_count;