| Message ID | 20200721123154.5302-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/isa: Allow 8/16/32 bit access on ISA bus after CVE-2020-13754 fix | expand |
On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > To fixes CVE-2020-13754, commit 5d971f9e67 refuses mismatching > sizes in memory_region_access_valid(). This gives troubles when > a device is on an ISA bus, because the CPU is free to use > 8/16-bit accesses on the bus (or up to 32-bit on EISA bus), > regardless what range is valid for the device. > > To allow surgical change for the 5.1 release, allow monkey > patching of the MemoryRegionOps (by making the MemoryRegion > field not const). This should be reverted after the release > and fixed in a more elegant manner. > > Fixes: 5d971f9e67 ('memory: Revert "accept mismatching sizes in memory_region_access_valid"') > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > include/exec/memory.h | 7 ++++++- > softmmu/memory.c | 12 ++++++++---- > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 9200b20130..84b5c617e2 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -1218,7 +1218,7 @@ static void memory_region_initfn(Object *obj) > MemoryRegion *mr = MEMORY_REGION(obj); > ObjectProperty *op; > > - mr->ops = &unassigned_mem_ops; > + mr->ops = g_memdup(&unassigned_mem_ops, sizeof(MemoryRegionOps)); > mr->enabled = true; > mr->romd_mode = true; > mr->global_locking = true; Don't you now need to g_memfree() mr->ops somewhere? Otherwise you've leaked it if the device which owned this MemoryRegion is hot-unplugged, I think. thanks -- PMM
On 7/21/20 2:33 PM, Peter Maydell wrote: > On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> To fixes CVE-2020-13754, commit 5d971f9e67 refuses mismatching >> sizes in memory_region_access_valid(). This gives troubles when >> a device is on an ISA bus, because the CPU is free to use >> 8/16-bit accesses on the bus (or up to 32-bit on EISA bus), >> regardless what range is valid for the device. >> >> To allow surgical change for the 5.1 release, allow monkey >> patching of the MemoryRegionOps (by making the MemoryRegion >> field not const). This should be reverted after the release >> and fixed in a more elegant manner. >> >> Fixes: 5d971f9e67 ('memory: Revert "accept mismatching sizes in memory_region_access_valid"') >> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> --- >> include/exec/memory.h | 7 ++++++- >> softmmu/memory.c | 12 ++++++++---- >> 2 files changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/softmmu/memory.c b/softmmu/memory.c >> index 9200b20130..84b5c617e2 100644 >> --- a/softmmu/memory.c >> +++ b/softmmu/memory.c >> @@ -1218,7 +1218,7 @@ static void memory_region_initfn(Object *obj) >> MemoryRegion *mr = MEMORY_REGION(obj); >> ObjectProperty *op; >> >> - mr->ops = &unassigned_mem_ops; >> + mr->ops = g_memdup(&unassigned_mem_ops, sizeof(MemoryRegionOps)); >> mr->enabled = true; >> mr->romd_mode = true; >> mr->global_locking = true; > > Don't you now need to g_memfree() mr->ops somewhere? Otherwise > you've leaked it if the device which owned this MemoryRegion > is hot-unplugged, I think. I haven't thinking of hot-unplug. I went with the simplest fix considering we are in freeze, and fixing the bug was more important that a leak at this point. I'll have a look at freeing this memory, hoping it is still less disruptive than a proper architectural change to fix this problem. > > thanks > -- PMM >
On Tue, 21 Jul 2020 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/21/20 2:33 PM, Peter Maydell wrote: > > On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > Don't you now need to g_memfree() mr->ops somewhere? Otherwise > > you've leaked it if the device which owned this MemoryRegion > > is hot-unplugged, I think. > > I haven't thinking of hot-unplug. I went with the simplest fix > considering we are in freeze, and fixing the bug was more > important that a leak at this point. > I'll have a look at freeing this memory, hoping it is still less > disruptive than a proper architectural change to fix this problem. Instead of g_memdup()ing the ops struct here, you could maybe do it in isa_register_ioport() instead. Then you don't need to worry about leaks because we know all ISA devices are not hotpluggable, and the ugliness is also a bit more constrained to the ISA code. (Coverity probably still thinks it's a leak though.) thanks -- PMM
On 7/21/20 2:49 PM, Peter Maydell wrote: > On Tue, 21 Jul 2020 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> >> On 7/21/20 2:33 PM, Peter Maydell wrote: >>> On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> Don't you now need to g_memfree() mr->ops somewhere? Otherwise >>> you've leaked it if the device which owned this MemoryRegion >>> is hot-unplugged, I think. >> >> I haven't thinking of hot-unplug. I went with the simplest fix >> considering we are in freeze, and fixing the bug was more >> important that a leak at this point. >> I'll have a look at freeing this memory, hoping it is still less >> disruptive than a proper architectural change to fix this problem. > > Instead of g_memdup()ing the ops struct here, you could maybe > do it in isa_register_ioport() instead. Then you don't need to > worry about leaks because we know all ISA devices are not > hotpluggable, and the ugliness is also a bit more constrained > to the ISA code. (Coverity probably still thinks it's a leak though.) I tried that first but got: memory.c: In function ‘memory_region_initfn’: memory.c:1221:13: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 1221 | mr->ops = &unassigned_mem_ops; | ^ memory.c: In function ‘memory_region_init_io’: memory.c:1488:13: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 1488 | mr->ops = ops ? ops : &unassigned_mem_ops; | ^ memory.c: In function ‘memory_region_init_ram_device_ptr’: memory.c:1625:13: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 1625 | mr->ops = &ram_device_mem_ops; | ^ memory.c: In function ‘memory_region_init_rom_device_nomigrate’: memory.c:1667:13: error: assignment discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers] 1667 | mr->ops = ops; | ^ Since this whole series is a kludge, I'm tempted to cast that to non-const but it starts to get really ugly... > > thanks > -- PMM >
On Tue, 21 Jul 2020 at 15:23, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > > On 7/21/20 2:49 PM, Peter Maydell wrote: > > On Tue, 21 Jul 2020 at 13:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >> > >> On 7/21/20 2:33 PM, Peter Maydell wrote: > >>> On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > >>> Don't you now need to g_memfree() mr->ops somewhere? Otherwise > >>> you've leaked it if the device which owned this MemoryRegion > >>> is hot-unplugged, I think. > >> > >> I haven't thinking of hot-unplug. I went with the simplest fix > >> considering we are in freeze, and fixing the bug was more > >> important that a leak at this point. > >> I'll have a look at freeing this memory, hoping it is still less > >> disruptive than a proper architectural change to fix this problem. > > > > Instead of g_memdup()ing the ops struct here, you could maybe > > do it in isa_register_ioport() instead. Then you don't need to > > worry about leaks because we know all ISA devices are not > > hotpluggable, and the ugliness is also a bit more constrained > > to the ISA code. (Coverity probably still thinks it's a leak though.) > > I tried that first but got: > > memory.c: In function ‘memory_region_initfn’: > memory.c:1221:13: error: assignment discards ‘const’ qualifier from > pointer target type [-Werror=discarded-qualifiers] > 1221 | mr->ops = &unassigned_mem_ops; > | ^ > memory.c: In function ‘memory_region_init_io’: > memory.c:1488:13: error: assignment discards ‘const’ qualifier from > pointer target type [-Werror=discarded-qualifiers] > 1488 | mr->ops = ops ? ops : &unassigned_mem_ops; > | ^ > memory.c: In function ‘memory_region_init_ram_device_ptr’: > memory.c:1625:13: error: assignment discards ‘const’ qualifier from > pointer target type [-Werror=discarded-qualifiers] > 1625 | mr->ops = &ram_device_mem_ops; > | ^ > memory.c: In function ‘memory_region_init_rom_device_nomigrate’: > memory.c:1667:13: error: assignment discards ‘const’ qualifier from > pointer target type [-Werror=discarded-qualifiers] > 1667 | mr->ops = ops; > | ^ I don't think you should need to change MemoryRegion::ops to non-const for this approach. Since you allocate the memory for the new ops struct in isa_register_ioport() you can set it up there before assigning it to mr->ops -- you're not trying to modify-in-place the mr->ops that's already there. thanks -- PMM
diff --git a/include/exec/memory.h b/include/exec/memory.h index 307e527835..22028af6b9 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -383,7 +383,12 @@ struct MemoryRegion { RAMBlock *ram_block; Object *owner; - const MemoryRegionOps *ops; + /* + * XXX this must be 'const' but to counter side effects of + * CVE-2020-13754, make it non-const to allow monkey patching + * the access sizes. Only allowed for QEMU release v5.1 :( + */ + MemoryRegionOps *ops; void *opaque; MemoryRegion *container; Int128 size; diff --git a/softmmu/memory.c b/softmmu/memory.c index 9200b20130..84b5c617e2 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -1218,7 +1218,7 @@ static void memory_region_initfn(Object *obj) MemoryRegion *mr = MEMORY_REGION(obj); ObjectProperty *op; - mr->ops = &unassigned_mem_ops; + mr->ops = g_memdup(&unassigned_mem_ops, sizeof(MemoryRegionOps)); mr->enabled = true; mr->romd_mode = true; mr->global_locking = true; @@ -1485,7 +1485,11 @@ void memory_region_init_io(MemoryRegion *mr, uint64_t size) { memory_region_init(mr, owner, name, size); - mr->ops = ops ? ops : &unassigned_mem_ops; + if (ops) { + mr->ops = g_memdup(ops, sizeof(MemoryRegionOps)); + } else { + mr->ops = g_memdup(&unassigned_mem_ops, sizeof(MemoryRegionOps)); + } mr->opaque = opaque; mr->terminates = true; } @@ -1622,7 +1626,7 @@ void memory_region_init_ram_device_ptr(MemoryRegion *mr, mr->ram = true; mr->terminates = true; mr->ram_device = true; - mr->ops = &ram_device_mem_ops; + mr->ops = g_memdup(&ram_device_mem_ops, sizeof(MemoryRegionOps)); mr->opaque = mr; mr->destructor = memory_region_destructor_ram; mr->dirty_log_mask = tcg_enabled() ? (1 << DIRTY_MEMORY_CODE) : 0; @@ -1664,7 +1668,7 @@ void memory_region_init_rom_device_nomigrate(MemoryRegion *mr, Error *err = NULL; assert(ops); memory_region_init(mr, owner, name, size); - mr->ops = ops; + mr->ops = g_memdup(ops, sizeof(MemoryRegionOps)); mr->opaque = opaque; mr->terminates = true; mr->rom_device = true;
To fixes CVE-2020-13754, commit 5d971f9e67 refuses mismatching sizes in memory_region_access_valid(). This gives troubles when a device is on an ISA bus, because the CPU is free to use 8/16-bit accesses on the bus (or up to 32-bit on EISA bus), regardless what range is valid for the device. To allow surgical change for the 5.1 release, allow monkey patching of the MemoryRegionOps (by making the MemoryRegion field not const). This should be reverted after the release and fixed in a more elegant manner. Fixes: 5d971f9e67 ('memory: Revert "accept mismatching sizes in memory_region_access_valid"') Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- include/exec/memory.h | 7 ++++++- softmmu/memory.c | 12 ++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-)