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

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

[Christian A. Ehrhardt]

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

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)
     if (nvram) {
         /* Write the record into the slot */
         memcpy(nvram, exchange, record_length);
-        memset(nvram + record_length, exchange_length - record_length, 0xFF);
+        memset(nvram + record_length, 0xFF, exchange_length - record_length);
         /* If a new record, increment the record_count */
         if (!record_found) {
             uint32_t record_count;
-- 
2.34.1

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

[Philippe Mathieu-Daudé]

On 19/10/22 21:15, Christian A. Ehrhardt wrote:
> Fix memset argument order: The second argument is
> the value, the length goes last.
> 
> 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)
>       if (nvram) {
>           /* Write the record into the slot */
>           memcpy(nvram, exchange, record_length);
> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
Ouch

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Eric DeVolder]

On 10/19/22 14:37, Philippe Mathieu-Daudé wrote:
> On 19/10/22 21:15, Christian A. Ehrhardt wrote:
>> Fix memset argument order: The second argument is
>> the value, the length goes last.
>>
>> 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)
>>       if (nvram) {
>>           /* Write the record into the slot */
>>           memcpy(nvram, exchange, record_length);
>> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
>> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
> Ouch
Sheesh, I'd hate to be that guy...

Reviewed-by: Eric DeVolder <eric.devolder@oracle.com>
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

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;

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

[Christian A. Ehrhardt]

Hi Markus,

On Thu, Oct 20, 2022 at 08:14:32AM +0200, 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?

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".

> > 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>
> >          /* Write the record into the slot */

[ ... ]

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

Incorrect and insufficient data is written.

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

Buffer overrun with well known potentially catastrophic consequences.
Additionally, incorrect data is used for the padding.

      regards    Christian

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

[Markus Armbruster]

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

> Hi Markus,
>
> On Thu, Oct 20, 2022 at 08:14:32AM +0200, 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?
>
> 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".
>
>> > 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>
>> >          /* Write the record into the slot */
>
> [ ... ]
>
>> 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?
>
> Incorrect and insufficient data is written.
>
>> If 0xFF > exchange_length - record_length, we write beyond the end of
>> the buffer.  Impact?
>
> Buffer overrun with well known potentially catastrophic consequences.
> Additionally, incorrect data is used for the padding.

Is record_length controlled by the guest?

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

[Christian A. Ehrhardt]

On Fri, Oct 21, 2022 at 06:22:50AM +0200, Markus Armbruster wrote:
> "Christian A. Ehrhardt" <lk@c--e.de> writes:
> 
> > Hi Markus,
> >
> > On Thu, Oct 20, 2022 at 08:14:32AM +0200, 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?
> >
> > 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".
> >
> >> > 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>
> >> >          /* Write the record into the slot */
> >
> > [ ... ]
> >
> >> 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?
> >
> > Incorrect and insufficient data is written.
> >
> >> If 0xFF > exchange_length - record_length, we write beyond the end of
> >> the buffer.  Impact?
> >
> > Buffer overrun with well known potentially catastrophic consequences.
> > Additionally, incorrect data is used for the padding.
> 
> Is record_length controlled by the guest?

Yes, it is taken from the CPER header in the exchange store.

     regards   Christian

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

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

[Alexander Bulekov]

On 221019 2115, Christian A. Ehrhardt wrote:
> Fix memset argument order: The second argument is
> the value, the length goes last.
> 
> 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)
>      if (nvram) {
>          /* Write the record into the slot */
>          memcpy(nvram, exchange, record_length);
> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);

Hi, 
I'm running the fuzzer over this code. So far, it hasn't complained
about this particular memset call, but it has crashed on the memcpy,
directly preceding it. I think the record_length checks might be
insufficient. I made an issue/reproducer:
https://gitlab.com/qemu-project/qemu/-/issues/1268

In that particular case, record_length is ffffff00 and passes the
checks. I'll keep an eye on the fuzzer to see if it will be able to
crash the memset.

For this patch:
Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

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

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

[Alexander Bulekov]

On 221021 1505, Alexander Bulekov wrote:
> On 221019 2115, Christian A. Ehrhardt wrote:
> > Fix memset argument order: The second argument is
> > the value, the length goes last.
> > 
> > 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)
> >      if (nvram) {
> >          /* Write the record into the slot */
> >          memcpy(nvram, exchange, record_length);
> > -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> > +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
> 
> Hi, 
> I'm running the fuzzer over this code. So far, it hasn't complained
> about this particular memset call, but it has crashed on the memcpy,
> directly preceding it. I think the record_length checks might be
> insufficient. I made an issue/reproducer:
> https://gitlab.com/qemu-project/qemu/-/issues/1268
> 
> In that particular case, record_length is ffffff00 and passes the
> checks. I'll keep an eye on the fuzzer to see if it will be able to
> crash the memset.

Here's a testcase for the memset issue:

cat << EOF | ./qemu-system-i386 -display none -machine accel=qtest, -m \
512M -object memory-backend-ram,id=erstnvram,size=0x10000 -device \
acpi-erst,memdev=erstnvram -nodefaults -qtest stdio
outl 0xcf8 0x80001010
outl 0xcfc 0xe0000000
outl 0xcf8 0x80001014
outl 0xcfc 0xe0002000
outl 0xcf8 0x80001004
outw 0xcfc 0x02
write 0xe0000008 0x1 0x9c
write 0xe0002015 0x1 0x01
write 0xe0002066 0x1 0x02
write 0xe0000000 0x1 0x05
write 0xe0002066 0x1 0x04
write 0xe0000000 0x1 0x05
write 0xe0002066 0x1 0x01
write 0xe0000000 0x1 0x05
write 0xe0002065 0x1 0x01
write 0xe0000000 0x1 0x05
write 0xe0002066 0x1 0x02
write 0xe0000000 0x1 0x05
write 0xe0002066 0x1 0x03
write 0xe0000000 0x1 0x05
write 0xe0002015 0x1 0x20
write 0xe0002066 0x1 0x00
write 0xe0000000 0x1 0x05
EOF

> 
> For this patch:
> Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> 
> >          /* If a new record, increment the record_count */
> >          if (!record_found) {
> >              uint32_t record_count;
> > -- 
> > 2.34.1
> > 
> > 

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

[Christian A. Ehrhardt]

Hi,

On Sat, Oct 22, 2022 at 01:37:27AM -0400, Alexander Bulekov wrote:
> On 221021 1505, Alexander Bulekov wrote:
> > On 221019 2115, Christian A. Ehrhardt wrote:
> > > Fix memset argument order: The second argument is
> > > the value, the length goes last.
> > > 
> > > 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)
> > >      if (nvram) {
> > >          /* Write the record into the slot */
> > >          memcpy(nvram, exchange, record_length);
> > > -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> > > +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
> > 
> > Hi, 
> > I'm running the fuzzer over this code. So far, it hasn't complained
> > about this particular memset call, but it has crashed on the memcpy,
> > directly preceding it. I think the record_length checks might be
> > insufficient. I made an issue/reproducer:
> > https://gitlab.com/qemu-project/qemu/-/issues/1268
> > 
> > In that particular case, record_length is ffffff00 and passes the
> > checks. I'll keep an eye on the fuzzer to see if it will be able to
> > crash the memset.
> 
> Here's a testcase for the memset issue:

Looks like this check in {read,write}_erst_record():
|   if ((s->record_offset + record_length) > exchange_length) {
|       return STATUS_FAILED;
|   }

has an integer overrun and should be re-written as:
|   if (record_length > exchange_length - s->record_offset) {
|       return STATUS_FAILED;
|   }

       regards    Christian

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

[Alexander Bulekov]

On 221023 1637, Christian A. Ehrhardt wrote:
> 
> Hi,
> 
> On Sat, Oct 22, 2022 at 01:37:27AM -0400, Alexander Bulekov wrote:
> > On 221021 1505, Alexander Bulekov wrote:
> > > On 221019 2115, Christian A. Ehrhardt wrote:
> > > > Fix memset argument order: The second argument is
> > > > the value, the length goes last.
> > > > 
> > > > 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)
> > > >      if (nvram) {
> > > >          /* Write the record into the slot */
> > > >          memcpy(nvram, exchange, record_length);
> > > > -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> > > > +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
> > > 
> > > Hi, 
> > > I'm running the fuzzer over this code. So far, it hasn't complained
> > > about this particular memset call, but it has crashed on the memcpy,
> > > directly preceding it. I think the record_length checks might be
> > > insufficient. I made an issue/reproducer:
> > > https://gitlab.com/qemu-project/qemu/-/issues/1268
> > > 
> > > In that particular case, record_length is ffffff00 and passes the
> > > checks. I'll keep an eye on the fuzzer to see if it will be able to
> > > crash the memset.
> > 
> > Here's a testcase for the memset issue:
> 
> Looks like this check in {read,write}_erst_record():
> |   if ((s->record_offset + record_length) > exchange_length) {
> |       return STATUS_FAILED;
> |   }
> 
> has an integer overrun and should be re-written as:
> |   if (record_length > exchange_length - s->record_offset) {
> |       return STATUS_FAILED;
> |   }
> 
>        regards    Christian

Hi Christian,
With this change applied (along with the memset fix), the fuzzer doesn't
find any crashes.
Thanks
-Alex

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

[Michael S. Tsirkin]

On Mon, Oct 24, 2022 at 09:20:40AM -0400, Alexander Bulekov wrote:
> On 221023 1637, Christian A. Ehrhardt wrote:
> > 
> > Hi,
> > 
> > On Sat, Oct 22, 2022 at 01:37:27AM -0400, Alexander Bulekov wrote:
> > > On 221021 1505, Alexander Bulekov wrote:
> > > > On 221019 2115, Christian A. Ehrhardt wrote:
> > > > > Fix memset argument order: The second argument is
> > > > > the value, the length goes last.
> > > > > 
> > > > > 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)
> > > > >      if (nvram) {
> > > > >          /* Write the record into the slot */
> > > > >          memcpy(nvram, exchange, record_length);
> > > > > -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> > > > > +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
> > > > 
> > > > Hi, 
> > > > I'm running the fuzzer over this code. So far, it hasn't complained
> > > > about this particular memset call, but it has crashed on the memcpy,
> > > > directly preceding it. I think the record_length checks might be
> > > > insufficient. I made an issue/reproducer:
> > > > https://gitlab.com/qemu-project/qemu/-/issues/1268
> > > > 
> > > > In that particular case, record_length is ffffff00 and passes the
> > > > checks. I'll keep an eye on the fuzzer to see if it will be able to
> > > > crash the memset.
> > > 
> > > Here's a testcase for the memset issue:
> > 
> > Looks like this check in {read,write}_erst_record():
> > |   if ((s->record_offset + record_length) > exchange_length) {
> > |       return STATUS_FAILED;
> > |   }
> > 
> > has an integer overrun and should be re-written as:
> > |   if (record_length > exchange_length - s->record_offset) {
> > |       return STATUS_FAILED;
> > |   }
> > 
> >        regards    Christian
> 
> Hi Christian,
> With this change applied (along with the memset fix), the fuzzer doesn't
> find any crashes.
> Thanks
> -Alex

Thanks!
Christian, are you doing to be sending a combined patch for the whole
issue? If you do pls add Tested-by tags as appropriate. Thanks!

-- 
MST

[PATCH v2] hw/acpi/erst.c: Fix memory handling issues

[Christian A. Ehrhardt]

- Fix memset argument order: The second argument is
  the value, the length goes last.
- Fix an integer overflow reported by Alexander Bulekov.

Both issues allow the guest to overrun the host buffer
allocated for the ERST memory device.

Cc: Eric DeVolder <eric.devolder@oracle.com
Cc: Alexander Bulekov <alxndr@bu.edu>
Cc: qemu-stable@nongnu.org
Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
 hw/acpi/erst.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
index df856b2669..aefcc03ad6 100644
--- a/hw/acpi/erst.c
+++ b/hw/acpi/erst.c
@@ -635,7 +635,7 @@ static unsigned read_erst_record(ERSTDeviceState *s)
         if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
             rc = STATUS_FAILED;
         }
-        if ((s->record_offset + record_length) > exchange_length) {
+        if (record_length > exchange_length - s->record_offset) {
             rc = STATUS_FAILED;
         }
         /* If all is ok, copy the record to the exchange buffer */
@@ -684,7 +684,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
     if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
         return STATUS_FAILED;
     }
-    if ((s->record_offset + record_length) > exchange_length) {
+    if (record_length > exchange_length - s->record_offset) {
         return STATUS_FAILED;
     }
 
@@ -716,7 +716,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
     if (nvram) {
         /* Write the record into the slot */
         memcpy(nvram, exchange, record_length);
-        memset(nvram + record_length, exchange_length - record_length, 0xFF);
+        memset(nvram + record_length, 0xFF, exchange_length - record_length);
         /* If a new record, increment the record_count */
         if (!record_found) {
             uint32_t record_count;
-- 
2.34.1

Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues

[Alexander Bulekov]

On 221024 1742, Christian A. Ehrhardt wrote:
> - Fix memset argument order: The second argument is
>   the value, the length goes last.
> - Fix an integer overflow reported by Alexander Bulekov.
> 
> Both issues allow the guest to overrun the host buffer
> allocated for the ERST memory device.
> 
> Cc: Eric DeVolder <eric.devolder@oracle.com
> Cc: Alexander Bulekov <alxndr@bu.edu>
> Cc: qemu-stable@nongnu.org
> Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
> Tested-by: Alexander Bulekov <alxndr@bu.edu>

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> ---
>  hw/acpi/erst.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index df856b2669..aefcc03ad6 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -635,7 +635,7 @@ static unsigned read_erst_record(ERSTDeviceState *s)
>          if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
>              rc = STATUS_FAILED;
>          }
> -        if ((s->record_offset + record_length) > exchange_length) {
> +        if (record_length > exchange_length - s->record_offset) {
>              rc = STATUS_FAILED;
>          }
>          /* If all is ok, copy the record to the exchange buffer */
> @@ -684,7 +684,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
>      if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
>          return STATUS_FAILED;
>      }
> -    if ((s->record_offset + record_length) > exchange_length) {
> +    if (record_length > exchange_length - s->record_offset) {
>          return STATUS_FAILED;
>      }
>  
> @@ -716,7 +716,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
>      if (nvram) {
>          /* Write the record into the slot */
>          memcpy(nvram, exchange, record_length);
> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
>          /* If a new record, increment the record_count */
>          if (!record_found) {
>              uint32_t record_count;
> -- 
> 2.34.1
> 
> 

Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues

[Alexander Bulekov]

On 221024 1224, Alexander Bulekov wrote:
> On 221024 1742, Christian A. Ehrhardt wrote:
> > - Fix memset argument order: The second argument is
> >   the value, the length goes last.
> > - Fix an integer overflow reported by Alexander Bulekov.
> > 
> > Both issues allow the guest to overrun the host buffer
> > allocated for the ERST memory device.
> > 
> > Cc: Eric DeVolder <eric.devolder@oracle.com
> > Cc: Alexander Bulekov <alxndr@bu.edu>
> > Cc: qemu-stable@nongnu.org
> > Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
> > Tested-by: Alexander Bulekov <alxndr@bu.edu>
> 

Also:
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1268

> Reviewed-by: Alexander Bulekov <alxndr@bu.edu>
> 
> > Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
> > ---
> >  hw/acpi/erst.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> > index df856b2669..aefcc03ad6 100644
> > --- a/hw/acpi/erst.c
> > +++ b/hw/acpi/erst.c
> > @@ -635,7 +635,7 @@ static unsigned read_erst_record(ERSTDeviceState *s)
> >          if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
> >              rc = STATUS_FAILED;
> >          }
> > -        if ((s->record_offset + record_length) > exchange_length) {
> > +        if (record_length > exchange_length - s->record_offset) {
> >              rc = STATUS_FAILED;
> >          }
> >          /* If all is ok, copy the record to the exchange buffer */
> > @@ -684,7 +684,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
> >      if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
> >          return STATUS_FAILED;
> >      }
> > -    if ((s->record_offset + record_length) > exchange_length) {
> > +    if (record_length > exchange_length - s->record_offset) {
> >          return STATUS_FAILED;
> >      }
> >  
> > @@ -716,7 +716,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
> >      if (nvram) {
> >          /* Write the record into the slot */
> >          memcpy(nvram, exchange, record_length);
> > -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> > +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
> >          /* If a new record, increment the record_count */
> >          if (!record_found) {
> >              uint32_t record_count;
> > -- 
> > 2.34.1
> > 
> > 

Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues

[Eric DeVolder]

On 10/24/22 10:42, Christian A. Ehrhardt wrote:
> - Fix memset argument order: The second argument is
>    the value, the length goes last.
> - Fix an integer overflow reported by Alexander Bulekov.
> 
> Both issues allow the guest to overrun the host buffer
> allocated for the ERST memory device.
> 
> Cc: Eric DeVolder <eric.devolder@oracle.com
> Cc: Alexander Bulekov <alxndr@bu.edu>
> Cc: qemu-stable@nongnu.org
> Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>

Reviewed-by: Eric DeVolder <eric.devolder@oracle.com>

Thanks Christian!
eric

> ---
>   hw/acpi/erst.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index df856b2669..aefcc03ad6 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -635,7 +635,7 @@ static unsigned read_erst_record(ERSTDeviceState *s)
>           if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
>               rc = STATUS_FAILED;
>           }
> -        if ((s->record_offset + record_length) > exchange_length) {
> +        if (record_length > exchange_length - s->record_offset) {
>               rc = STATUS_FAILED;
>           }
>           /* If all is ok, copy the record to the exchange buffer */
> @@ -684,7 +684,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
>       if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
>           return STATUS_FAILED;
>       }
> -    if ((s->record_offset + record_length) > exchange_length) {
> +    if (record_length > exchange_length - s->record_offset) {
>           return STATUS_FAILED;
>       }
>   
> @@ -716,7 +716,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
>       if (nvram) {
>           /* Write the record into the slot */
>           memcpy(nvram, exchange, record_length);
> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
>           /* If a new record, increment the record_count */
>           if (!record_found) {
>               uint32_t record_count;
> 

Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues

[Michael S. Tsirkin]

On Mon, Oct 24, 2022 at 05:42:33PM +0200, Christian A. Ehrhardt wrote:
> - Fix memset argument order: The second argument is
>   the value, the length goes last.
> - Fix an integer overflow reported by Alexander Bulekov.
> 
> Both issues allow the guest to overrun the host buffer
> allocated for the ERST memory device.
> 
> Cc: Eric DeVolder <eric.devolder@oracle.com
> Cc: Alexander Bulekov <alxndr@bu.edu>
> Cc: qemu-stable@nongnu.org
> Fixes: f7e26ffa590 ("ACPI ERST: support for ACPI ERST feature")
> Tested-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>


queued, thanks!

> ---
>  hw/acpi/erst.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/erst.c b/hw/acpi/erst.c
> index df856b2669..aefcc03ad6 100644
> --- a/hw/acpi/erst.c
> +++ b/hw/acpi/erst.c
> @@ -635,7 +635,7 @@ static unsigned read_erst_record(ERSTDeviceState *s)
>          if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
>              rc = STATUS_FAILED;
>          }
> -        if ((s->record_offset + record_length) > exchange_length) {
> +        if (record_length > exchange_length - s->record_offset) {
>              rc = STATUS_FAILED;
>          }
>          /* If all is ok, copy the record to the exchange buffer */
> @@ -684,7 +684,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
>      if (record_length < UEFI_CPER_RECORD_MIN_SIZE) {
>          return STATUS_FAILED;
>      }
> -    if ((s->record_offset + record_length) > exchange_length) {
> +    if (record_length > exchange_length - s->record_offset) {
>          return STATUS_FAILED;
>      }
>  
> @@ -716,7 +716,7 @@ static unsigned write_erst_record(ERSTDeviceState *s)
>      if (nvram) {
>          /* Write the record into the slot */
>          memcpy(nvram, exchange, record_length);
> -        memset(nvram + record_length, exchange_length - record_length, 0xFF);
> +        memset(nvram + record_length, 0xFF, exchange_length - record_length);
>          /* If a new record, increment the record_count */
>          if (!record_found) {
>              uint32_t record_count;
> -- 
> 2.34.1