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