| Message ID | 1536755529-2709-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | memory region: check the old.mmio.read status | expand |
On 12 September 2018 at 13:32, Li Qiang <liq3ea@gmail.com> wrote: > To avoid NULL-deref for the devices without read callbacks > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > --- > 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
Peter Maydell <peter.maydell@linaro.org> 于2018年9月12日周三 下午8:55写道： > On 12 September 2018 at 13:32, Li Qiang <liq3ea@gmail.com> wrote: > > To avoid NULL-deref for the devices without read callbacks > > > > Signed-off-by: Li Qiang <liq3ea@gmail.com> > > --- > > 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 >
On 09/12/18 14:54, Peter Maydell wrote: > On 12 September 2018 at 13:32, Li Qiang <liq3ea@gmail.com> wrote: >> To avoid NULL-deref for the devices without read callbacks >> >> Signed-off-by: Li Qiang <liq3ea@gmail.com> >> --- >> 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/84da6f02-1f60-4bc7-92da-6a7f74deded3@redhat.com>.) 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
On 09/12/18 16:28, Li Qiang wrote: > Peter Maydell <peter.maydell@linaro.org> 于2018年9月12日周三 下午8:55写道： > >> On 12 September 2018 at 13:32, Li Qiang <liq3ea@gmail.com> wrote: >>> To avoid NULL-deref for the devices without read callbacks >>> >>> Signed-off-by: Li Qiang <liq3ea@gmail.com> >>> --- >>> 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
On 12 September 2018 at 18:43, Laszlo Ersek <lersek@redhat.com> 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/84da6f02-1f60-4bc7-92da-6a7f74deded3@redhat.com>.) > 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
Peter Maydell <peter.maydell@linaro.org> 于2018年9月13日周四 上午8:31写道： > On 12 September 2018 at 18:43, Laszlo Ersek <lersek@redhat.com> 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/84da6f02-1f60-4bc7-92da-6a7f74deded3@redhat.com > >.) > > 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 >
On 13/09/18 01:31, Peter Maydell wrote: > On 12 September 2018 at 18:43, Laszlo Ersek <lersek@redhat.com> 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/84da6f02-1f60-4bc7-92da-6a7f74deded3@redhat.com>.) >> 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.
On 13 September 2018 at 05:31, Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> 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
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);
To avoid NULL-deref for the devices without read callbacks Signed-off-by: Li Qiang <liq3ea@gmail.com> --- memory.c | 4 ++++ 1 file changed, 4 insertions(+)