"Christian A. Ehrhardt" <l...@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.devol...@oracle.com>
> Cc: qemu-sta...@nongnu.org
> Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
> Signed-off-by: Christian A. Ehrhardt <l...@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;
