[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/6] pc: fix possible NULL pointer dereferenc
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-devel] [PATCH v3 1/6] pc: fix possible NULL pointer dereference in pc_machine_get_device_memory_region_size() |
Date: |
Tue, 28 May 2019 15:46:23 +0200 |
On Mon, 27 May 2019 18:36:25 +0200
Markus Armbruster <address@hidden> wrote:
> Igor Mammedov <address@hidden> writes:
>
> > QEMU will crash when device-memory-region-size property is read if
> > ms->device_memory
> > wasn't initialized yet (ex: property being inspected during preconfig
> > time).
>
> Reproduced:
>
> $ qemu-system-x86_64 -nodefaults -S -display none -preconfig -qmp stdio
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 0, "major": 4},
> "package": "v4.0.0-828-ga7b21f6762"}, "capabilities": ["oob"]}}
> {"execute": "qmp_capabilities"}
> {"return": {}}
> {"execute": "qom-get", "arguments": {"path": "/machine", "property":
> "device-memory-region-size"}}
> Segmentation fault (core dumped)
>
> First time I started looking at this series, I went "I'll need a
> reproducer to fully understand what's up, and I don't feel like finding
> one now; next series, please". Second time, I had to spend a few
> minutes on the reproducer. Wasn't hard, since you provided a clue.
> Still: make review easy, include a reproducer whenever you can.
sure
>
> > Instead of crashing return 0 if ms->device_memory hasn't been initialized.
> >
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> > hw/i386/pc.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> > index d98b737..de91e90 100644
> > --- a/hw/i386/pc.c
> > +++ b/hw/i386/pc.c
> > @@ -2461,7 +2461,11 @@ pc_machine_get_device_memory_region_size(Object
> > *obj, Visitor *v,
> > Error **errp)
> > {
> > MachineState *ms = MACHINE(obj);
> > - int64_t value = memory_region_size(&ms->device_memory->mr);
> > + int64_t value = 0;
> > +
> > + if (ms->device_memory) {
> > + memory_region_size(&ms->device_memory->mr);
> > + }
> >
> > visit_type_int(v, name, &value, errp);
> > }
>
> This makes qom-get return 0 for the size of memory that doesn't exist,
> yet.
>
> A possible alternative would be setting an error.
>
> Opinions?
We don't have a notion of property not set in QOM, so a code that
will receive a text based error will have to parse it (horrible idea)
to avoid generation of related ACPI parts.
In case of not enabled memory hotplug, PC_MACHINE_DEVMEM_REGION_SIZE == 0
is valid value and it's what's expected by other code.
- [Qemu-devel] [PATCH v3 0/6] numa: deprecate '-numa node, mem' and default memory distribution, Igor Mammedov, 2019/05/17
- [Qemu-devel] [PATCH v3 1/6] pc: fix possible NULL pointer dereference in pc_machine_get_device_memory_region_size(), Igor Mammedov, 2019/05/17
- [Qemu-devel] [PATCH v3 2/6] qmp: make "qom-list-properties" show initial property values, Igor Mammedov, 2019/05/17
- [Qemu-devel] [PATCH v3 3/6] qmp: qmp_qom_list_properties(): ignore empty string options, Igor Mammedov, 2019/05/17
- [Qemu-devel] [PATCH v3 4/6] numa: introduce "numa-mem-supported" machine property, Igor Mammedov, 2019/05/17
- [Qemu-devel] [PATCH v3 5/6] numa: deprecate 'mem' parameter of '-numa node' option, Igor Mammedov, 2019/05/17
- [Qemu-devel] [PATCH v3 6/6] numa: deprecate implict memory distribution between nodes, Igor Mammedov, 2019/05/17