memory region: check the old.mmio.read status

Commit Message

Li Qiang Sept. 12, 2018, 12:32 p.m. UTC

To avoid NULL-deref for the devices without read callbacks

Signed-off-by: Li Qiang <[e-mail]>
---
 memory.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Peter Maydell Sept. 12, 2018, 12:54 p.m. UTC | #1

On 12 September 2018 at 13:32, Li Qiang <[e-mail]> wrote:
> To avoid NULL-deref for the devices without read callbacks
>
> Signed-off-by: Li Qiang <[e-mail]>
> ---
>  memory.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/memory.c b/memory.c
> index 9b73892768..48d025426b 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -406,6 +406,10 @@ static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>  {
>      uint64_t tmp;
>
> +    if (!mr->ops->old_mmio.read[ctz32(size)]) {
> +        return MEMTX_DECODE_ERROR;
> +    }
> +
>      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
>      if (mr->subpage) {
>          trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
> --
> 2.11.0
>

There's patches on-list which drop the old_mmio field from the MemoryRegion
struct entirely, so I think this patch as it stands is obsolete.

Currently our semantics are "you must provide both read and write, even
if one of them just always returns 0 / does nothing / returns an error".
We could probably reasonably assert this at the point when the
MemoryRegionOps is registered.

thanks
-- PMM

Li Qiang Sept. 12, 2018, 2:28 p.m. UTC | #2

Peter Maydell <[e-mail]> 于2018年9月12日周三 下午8:55写道：

> On 12 September 2018 at 13:32, Li Qiang <[e-mail]> wrote:
> > To avoid NULL-deref for the devices without read callbacks
> >
> > Signed-off-by: Li Qiang <[e-mail]>
> > ---
> >  memory.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/memory.c b/memory.c
> > index 9b73892768..48d025426b 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -406,6 +406,10 @@ static MemTxResult
> memory_region_oldmmio_read_accessor(MemoryRegion *mr,
> >  {
> >      uint64_t tmp;
> >
> > +    if (!mr->ops->old_mmio.read[ctz32(size)]) {
> > +        return MEMTX_DECODE_ERROR;
> > +    }
> > +
> >      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
> >      if (mr->subpage) {
> >          trace_memory_region_subpage_read(get_cpu_index(), mr, addr,
> tmp, size);
> > --
> > 2.11.0
> >
>
> There's patches on-list which drop the old_mmio field from the MemoryRegion
> struct entirely, so I think this patch as it stands is obsolete.
>
> Currently our semantics are "you must provide both read and write, even
> if one of them just always returns 0 / does nothing / returns an error".
> We could probably reasonably assert this at the point when the
> MemoryRegionOps is registered.
>

This patch is sent as the results of this thread:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01332.html

So I think  I should send a path set to add all the missing read function
as Laszlo Ersek points
in the above thread discussion, right?

Thanks,
Li Qiang




>
> thanks
> -- PMM
>

Laszlo Ersek Sept. 12, 2018, 5:43 p.m. UTC | #3

On 09/12/18 14:54, Peter Maydell wrote:
> On 12 September 2018 at 13:32, Li Qiang <[e-mail]> wrote:
>> To avoid NULL-deref for the devices without read callbacks
>>
>> Signed-off-by: Li Qiang <[e-mail]>
>> ---
>>  memory.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/memory.c b/memory.c
>> index 9b73892768..48d025426b 100644
>> --- a/memory.c
>> +++ b/memory.c
>> @@ -406,6 +406,10 @@ static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>>  {
>>      uint64_t tmp;
>>
>> +    if (!mr->ops->old_mmio.read[ctz32(size)]) {
>> +        return MEMTX_DECODE_ERROR;
>> +    }
>> +
>>      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
>>      if (mr->subpage) {
>>          trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);
>> --
>> 2.11.0
>>
> 
> There's patches on-list which drop the old_mmio field from the MemoryRegion
> struct entirely, so I think this patch as it stands is obsolete.
> 
> Currently our semantics are "you must provide both read and write, even
> if one of them just always returns 0 / does nothing / returns an error".

That's new to me. Has this always been the case? There are several
static MemoryRegionOps structures that don't conform. (See the end of my
other email:
<http://mid.mail-archive.com/[e-mail]>.)
Beyond the one that Li Qiang reported directly ("fw_cfg_ctl_mem_read").

Are all of those ops guest-triggerable QEMU crashers?

> We could probably reasonably assert this at the point when the
> MemoryRegionOps is registered.

Apparently, we should have...

Thanks,
Laszlo

Laszlo Ersek Sept. 12, 2018, 5:45 p.m. UTC | #4

On 09/12/18 16:28, Li Qiang wrote:
> Peter Maydell <[e-mail]> 于2018年9月12日周三 下午8:55写道：
> 
>> On 12 September 2018 at 13:32, Li Qiang <[e-mail]> wrote:
>>> To avoid NULL-deref for the devices without read callbacks
>>>
>>> Signed-off-by: Li Qiang <[e-mail]>
>>> ---
>>>  memory.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/memory.c b/memory.c
>>> index 9b73892768..48d025426b 100644
>>> --- a/memory.c
>>> +++ b/memory.c
>>> @@ -406,6 +406,10 @@ static MemTxResult
>> memory_region_oldmmio_read_accessor(MemoryRegion *mr,
>>>  {
>>>      uint64_t tmp;
>>>
>>> +    if (!mr->ops->old_mmio.read[ctz32(size)]) {
>>> +        return MEMTX_DECODE_ERROR;
>>> +    }
>>> +
>>>      tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
>>>      if (mr->subpage) {
>>>          trace_memory_region_subpage_read(get_cpu_index(), mr, addr,
>> tmp, size);
>>> --
>>> 2.11.0
>>>
>>
>> There's patches on-list which drop the old_mmio field from the MemoryRegion
>> struct entirely, so I think this patch as it stands is obsolete.
>>
>> Currently our semantics are "you must provide both read and write, even
>> if one of them just always returns 0 / does nothing / returns an error".
>> We could probably reasonably assert this at the point when the
>> MemoryRegionOps is registered.
>>
> 
> This patch is sent as the results of this thread:
> -->https://lists.gnu.org/archive/html/qemu-devel/2018-09/msg01332.html
> 
> So I think  I should send a path set to add all the missing read function
> as Laszlo Ersek points
> in the above thread discussion, right?

Can we introduce a central utility function at least (for a no-op read
returning 0), and initialize the read callbacks in question with the
address of that function?

Thanks,
Laszlo

Peter Maydell Sept. 13, 2018, 12:31 a.m. UTC | #5

On 12 September 2018 at 18:43, Laszlo Ersek <[e-mail]> wrote:
> On 09/12/18 14:54, Peter Maydell wrote:
>> There's patches on-list which drop the old_mmio field from the MemoryRegion
>> struct entirely, so I think this patch as it stands is obsolete.
>>
>> Currently our semantics are "you must provide both read and write, even
>> if one of them just always returns 0 / does nothing / returns an error".
>
> That's new to me. Has this always been the case?

Pretty sure it has, yes, because the code assumes that if you can
get a guest read then your MemoryRegion provides an accessor for it.
If your guest never actually tries to do a read then of course we'll
never notice...

> There are several
> static MemoryRegionOps structures that don't conform. (See the end of my
> other email:
> <http://mid.mail-archive.com/[e-mail]>.)
> Beyond the one that Li Qiang reported directly ("fw_cfg_ctl_mem_read").
>
> Are all of those ops guest-triggerable QEMU crashers?

Some of them are special cases like the notdirty-memory one where
reads always go to host RAM rather than taking the slow path via
the io accessor. But others are probably guest crashers.

>> We could probably reasonably assert this at the point when the
>> MemoryRegionOps is registered.
>
> Apparently, we should have...

Yeah. Or we could define a default for if there's no read function,
which I guess should be the same as what we do if
memory_region_access_valid() fails. If we want that then the
simplest thing is for memory_region_access_valid() itself to
check that at least one of the accessor functions exists and
return false if none do. (But as I mention above we should get
all the "old_mmio is going away" patches in first and base the
change on that, or there'll be a conflict.)

thanks
-- PMM

Li Qiang Sept. 13, 2018, 1:36 a.m. UTC | #6

Peter Maydell <[e-mail]> 于2018年9月13日周四 上午8:31写道：

> On 12 September 2018 at 18:43, Laszlo Ersek <[e-mail]> wrote:
> > On 09/12/18 14:54, Peter Maydell wrote:
> >> There's patches on-list which drop the old_mmio field from the
> MemoryRegion
> >> struct entirely, so I think this patch as it stands is obsolete.
> >>
> >> Currently our semantics are "you must provide both read and write, even
> >> if one of them just always returns 0 / does nothing / returns an error".
> >
> > That's new to me. Has this always been the case?
>
> Pretty sure it has, yes, because the code assumes that if you can
> get a guest read then your MemoryRegion provides an accessor for it.
> If your guest never actually tries to do a read then of course we'll
> never notice...
>
> > There are several
> > static MemoryRegionOps structures that don't conform. (See the end of my
> > other email:
> > <
> http://mid.mail-archive.com/[e-mail]
> >.)
> > Beyond the one that Li Qiang reported directly ("fw_cfg_ctl_mem_read").
> >
> > Are all of those ops guest-triggerable QEMU crashers?
>
> Some of them are special cases like the notdirty-memory one where
> reads always go to host RAM rather than taking the slow path via
> the io accessor. But others are probably guest crashers.
>
> >> We could probably reasonably assert this at the point when the
> >> MemoryRegionOps is registered.
> >
> > Apparently, we should have...
>
> Yeah. Or we could define a default for if there's no read function,
> which I guess should be the same as what we do if
> memory_region_access_valid() fails.

If we want that then the
> simplest thing is for memory_region_access_valid() itself to
> check that at least one of the accessor functions exists and
> return false if none do.


I thinks this proposal makes sense as every memory region write/read will
go to this path and also the device code can make no change.

Thanks,
Li Qiang

(But as I mention above we should get
> all the "old_mmio is going away" patches in first and base the
> change on that, or there'll be a conflict.)
>
>



> thanks
> -- PMM
>

Mark Cave-Ayland Sept. 13, 2018, 4:31 a.m. UTC | #7

On 13/09/18 01:31, Peter Maydell wrote:

> On 12 September 2018 at 18:43, Laszlo Ersek <[e-mail]> wrote:
>> On 09/12/18 14:54, Peter Maydell wrote:
>>> There's patches on-list which drop the old_mmio field from the MemoryRegion
>>> struct entirely, so I think this patch as it stands is obsolete.
>>>
>>> Currently our semantics are "you must provide both read and write, even
>>> if one of them just always returns 0 / does nothing / returns an error".
>>
>> That's new to me. Has this always been the case?
> 
> Pretty sure it has, yes, because the code assumes that if you can
> get a guest read then your MemoryRegion provides an accessor for it.
> If your guest never actually tries to do a read then of course we'll
> never notice...
> 
>> There are several
>> static MemoryRegionOps structures that don't conform. (See the end of my
>> other email:
>> <http://mid.mail-archive.com/[e-mail]>.)
>> Beyond the one that Li Qiang reported directly ("fw_cfg_ctl_mem_read").
>>
>> Are all of those ops guest-triggerable QEMU crashers?
> 
> Some of them are special cases like the notdirty-memory one where
> reads always go to host RAM rather than taking the slow path via
> the io accessor. But others are probably guest crashers.
> 
>>> We could probably reasonably assert this at the point when the
>>> MemoryRegionOps is registered.
>>
>> Apparently, we should have...
> 
> Yeah. Or we could define a default for if there's no read function,
> which I guess should be the same as what we do if
> memory_region_access_valid() fails. If we want that then the
> simplest thing is for memory_region_access_valid() itself to
> check that at least one of the accessor functions exists and
> return false if none do. (But as I mention above we should get
> all the "old_mmio is going away" patches in first and base the
> change on that, or there'll be a conflict.)

This sounds familiar to me. I remember whilst working on the Mac
uninorth patches I couldn't quite figure out why a simple change to the
PCI bridge IO address space started to cause some accesses to fail: it
was because the guest was issuing a periodic read to an address without
a MemoryRegion which was now failing with MEMTX_ERROR rather than the
returning 0 which was the previous behaviour.

The fix was ultimately to add an unassigned_io_ops to the parent
MemoryRegion which I found out was being sneakily added by the old code,
although it certainly had me scratching my head for a while...


ATB,

Mark.

Peter Maydell Sept. 13, 2018, 4:45 a.m. UTC | #8

On 13 September 2018 at 05:31, Mark Cave-Ayland
<[e-mail]> wrote:
> This sounds familiar to me. I remember whilst working on the Mac
> uninorth patches I couldn't quite figure out why a simple change to the
> PCI bridge IO address space started to cause some accesses to fail: it
> was because the guest was issuing a periodic read to an address without
> a MemoryRegion which was now failing with MEMTX_ERROR rather than the
> returning 0 which was the previous behaviour.

You may have been caught by changes in the handling of
unmapped-region accesses: historically we did read-as-zero/write-ignored,
which is some combination of "what x86 does" and the natural result
of not having support for flagging bus errors up to the CPU emulation.
Adding support for architectures that need bus errors to be reported
probably meant a change in the default at some point.

One thing we don't handle as cleanly as might be ideal is the case where
architecturally the CPU supports bus faults but the bus in an SoC or
board doesn't actually report unmapped accesses back to the CPU as
bus faults. You can model that by adding a suitable io accessor to the
relevant container MR, as you found, but it's a bit unobvious.

thanks
-- PMM

Patch

diff --git a/memory.c b/memory.c
index 9b73892768..48d025426b 100644
--- a/memory.c
+++ b/memory.c
@@ -406,6 +406,10 @@  static MemTxResult memory_region_oldmmio_read_accessor(MemoryRegion *mr,
 {
     uint64_t tmp;
 
+    if (!mr->ops->old_mmio.read[ctz32(size)]) {
+        return MEMTX_DECODE_ERROR;
+    }
+
     tmp = mr->ops->old_mmio.read[ctz32(size)](mr->opaque, addr);
     if (mr->subpage) {
         trace_memory_region_subpage_read(get_cpu_index(), mr, addr, tmp, size);