[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] hw/acpi/erst.c: Fix memset argument order
From: |
Michael S. Tsirkin |
Subject: |
Re: [PATCH] hw/acpi/erst.c: Fix memset argument order |
Date: |
Mon, 24 Oct 2022 10:04:18 -0400 |
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
- Re: [PATCH] hw/acpi/erst.c: Fix memset argument order, (continued)
- Re: [PATCH] hw/acpi/erst.c: Fix memset argument order, Alexander Bulekov, 2022/10/21
- Re: [PATCH] hw/acpi/erst.c: Fix memset argument order, Alexander Bulekov, 2022/10/22
- Re: [PATCH] hw/acpi/erst.c: Fix memset argument order, Christian A. Ehrhardt, 2022/10/23
- Re: [PATCH] hw/acpi/erst.c: Fix memset argument order, Alexander Bulekov, 2022/10/24
- Re: [PATCH] hw/acpi/erst.c: Fix memset argument order,
Michael S. Tsirkin <=
- [PATCH v2] hw/acpi/erst.c: Fix memory handling issues, Christian A. Ehrhardt, 2022/10/24
- Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues, Alexander Bulekov, 2022/10/24
- Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues, Alexander Bulekov, 2022/10/24
- Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues, Eric DeVolder, 2022/10/24
- Re: [PATCH v2] hw/acpi/erst.c: Fix memory handling issues, Michael S. Tsirkin, 2022/10/24