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

[Eric DeVolder]

On 10/20/22 01:14, Markus Armbruster wrote:
> "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.

Igor requested I use memcpy() so that this would work on EB and EL hosts.

>         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?

The purpose of the memset() is to ensure the slot does not contain any remnants of a previous 
record. There is no functional requirement for this; other than it was intended to prevent the 
possibility of leaking data.

If there were a previously deleted/overwritten record in that slot, then the tail of that record 
would remain. However, it still isn't visible upon the record read; it would only be visible by 
directly accessing the backing file/memory.

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

There are two cases here, if the record is stored in any slot but the last, then it has the 
opportunity to corrupt the next adjacent slot/record. Given that the CPER format places the magic 
'CPER' as the first 4 bytes, then I believe that upon next read of this corrupted record, it will be 
rejected as it does not have a valid CPER header.

If the record is the last in the backing storage, then it would attempt to write beyond the end. The 
backing store is memory mapped into the guest, so I believe that an attempt to write beyond the end 
will result in a segfault.

Previously stated: "Well, this is a memory error, i.e. the potential impact is
anything from silent data corruption to arbitrary code execution.
Phillipe described this accurately as "Ouch".

Yes, ouch. I had it correct until patch series v7 (of v15); not that that is 
helpful.

However, I do not see a path to arbitrary code execution. The erroneous memset() will write out a 
constant value (exchange_length - record_length) for 0xFF bytes.

In terms of current Linux real world impact, ERST is used as pstore backend and writes to pstore 
typically only happen on kernel panic, if configured. Furthermore, the systemd-pstore service 
attempts to keep the pstore empty, so that reduces the chances of pstore/ERST filling up and 
reaching that last slot that could cause a segfault.

ERST can be written by MCE, I think; not sure how relevant that is to guests.

eric

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