[RFC PATCH-for-5.1? v3 0/2] hw/isa: Allow 8/16/32 bit access on ISA bus after CVE-2020-13754 fix

[RFC PATCH-for-5.1? v3 0/2] hw/isa: Allow 8/16/32 bit access on ISA bus after CVE-2020-13754 fix

[Philippe Mathieu-Daudé]

Surgical attempt to fix the issue, as we are in freeze...

If such kludge is tolerable, it must be reverted first thing
after release.

Fixes:
- https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
- https://bugs.debian.org/964793
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964247
- https://bugs.launchpad.net/bugs/1886318

Philippe Mathieu-Daudé (2):
  memory: Allow monkey-patching MemoryRegion access sizes
  hw/isa/isa-bus: Ensure ISA I/O regions are 8/16/32-bit accessible

 include/exec/memory.h |  7 ++++++-
 hw/isa/isa-bus.c      | 14 ++++++++++++++
 softmmu/memory.c      | 12 ++++++++----
 3 files changed, 28 insertions(+), 5 deletions(-)

-- 
2.21.3

[RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Philippe Mathieu-Daudé]

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

[RFC PATCH-for-5.1? v3 2/2] hw/isa/isa-bus: Ensure ISA I/O regions are 8/16/32-bit accessible

[Philippe Mathieu-Daudé]

Since commit 5d971f9e67 we don't accept 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.

Monkey-patch the ISA device MemoryRegionOps to force it
to accepts 8/16/32-bit accesses. This should be reverted
after the release and fixed in a more elegant manner.

Related bug reports:

- https://lore.kernel.org/xen-devel/20200630170913.123646-1-anthony.perard@citrix.com/T/
- https://bugs.debian.org/964793
- https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=964247
- https://bugs.launchpad.net/bugs/1886318

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/isa-bus.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index 58fde178f9..c8aed2f55f 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -132,6 +132,20 @@ static inline void isa_init_ioport(ISADevice *dev, uint16_t ioport)
 
 void isa_register_ioport(ISADevice *dev, MemoryRegion *io, uint16_t start)
 {
+    if (io->ops->valid.min_access_size > 1 ||
+        io->ops->valid.max_access_size < 4) {
+        warn_report_once("Monkey-patching ISA I/O access sizes "
+                         "(side effect of CVE-2020-13754, only for QEMU v5.1)");
+        /*
+         * To be backward compatible with IBM-PC bus, ISA bus must accept
+         * 8-bit accesses.
+         */
+        io->ops->valid.min_access_size = 1;
+        /*
+         * EISA bus must accept 32-bit accesses.
+         */
+        io->ops->valid.max_access_size = 4;
+    }
     memory_region_add_subregion(isabus->address_space_io, start, io);
     isa_init_ioport(dev, start);
 }
-- 
2.21.3

Re: [RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Peter Maydell]

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

Re: [RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Philippe Mathieu-Daudé]

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
> 

Re: [RFC PATCH-for-5.1? v3 2/2] hw/isa/isa-bus: Ensure ISA I/O regions are 8/16/32-bit accessible

[Peter Maydell]

On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Since commit 5d971f9e67 we don't accept 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.
>
> Monkey-patch the ISA device MemoryRegionOps to force it
> to accepts 8/16/32-bit accesses. This should be reverted
> after the release and fixed in a more elegant manner.

Do we need something similar for isa_register_portio_list(),
or is that function OK ?

Do we have a view on what the 'more elegant manner' would look like?

thanks
-- PMM

Re: [RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Peter Maydell]

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

Re: [RFC PATCH-for-5.1? v3 2/2] hw/isa/isa-bus: Ensure ISA I/O regions are 8/16/32-bit accessible

[Philippe Mathieu-Daudé]

On 7/21/20 2:41 PM, Peter Maydell wrote:
> On Tue, 21 Jul 2020 at 13:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Since commit 5d971f9e67 we don't accept 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.
>>
>> Monkey-patch the ISA device MemoryRegionOps to force it
>> to accepts 8/16/32-bit accesses. This should be reverted
>> after the release and fixed in a more elegant manner.
> 
> Do we need something similar for isa_register_portio_list(),
> or is that function OK ?
> 
> Do we have a view on what the 'more elegant manner' would look like?

What I suggested on IRC is a isabus->address_space_io is not assigned
by the bus creator but created internally as a MemoryRegion container
accepting 8/16 (ISA bus) or 8/16/32-bit (EISA bus) accesses from the I/O
address space, and this MR uses memory::access_with_adjusted_size()
(or similar) to access the registered portio devices.

We already have isa_address_space_io() to access
isabus->address_space_io.

isa_bus_new() could takes an 'is_eisa' boolean argument to select
between the adjusting MR.

> 
> thanks
> -- PMM
> 

Re: [RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Philippe Mathieu-Daudé]

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
> 

Re: [RFC PATCH-for-5.1? v3 1/2] memory: Allow monkey-patching MemoryRegion access sizes

[Peter Maydell]

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