On 09/12/18 14:54, Peter Maydell wrote: > On 12 September 2018 at 13:32, Li Qiang <liq...@gmail.com> wrote: >> To avoid NULL-deref for the devices without read callbacks >> >> Signed-off-by: Li Qiang <liq...@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: <84da6f02-1f60-4bc7-92da-6a7f74deded3@redhat.com">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