Re: [PATCH] softmmu/memory: Validate {read, write}_with_attrs before calling

[Bin Meng]

On Mon, Sep 6, 2021 at 12:29 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 5 Sept 2021 at 16:40, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > {read,write}_with_attrs might be missing, and the codes currently do
> > not validate them before calling, which will cause segment fault.
> >
> > Fixes: 62a0db942dec ("memory: Remove old_mmio accessors")
> > Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>
> This 'fixes' tag doesn't seem quite right. Before that
> commit 62a0db942dec, we still required that a MemoryRegionOps
> provided some form of both read and write.

Did you mean before commit 62a0db942dec leaving MemoryRegionOps NULL
was not allowed, but things changed so that now it's possible to have
NULL MemoryRegionOps? If yes, then the commit id of "Fixes" should
probably go to the changes that allowed NULL memory ops. If not, I
don't think "Fixes" tag is wrong.

> It was never
> valid to leave all of the possible read function pointers NULL.
>
> What are the examples of devices which are deliberately leaving
> these pointers set to NULL?

No in-tree real examples. I just read the codes and found no
protection against the {read,write}_with_attrs ops.

> Last time this came up, we discussed the other option, which
> is to have memory_region_init assert that the MemoryRegionOps
> defines at least one valid read and one valid write pointer,
> so that these bugs get caught quickly rather than only if the
> guest accesses the device in the wrong way. I tend to think
> that that is better, because for any particular device
> it's not necessarily the right thing to return MEMTX_ERROR
> (maybe the right behaviour is "return 0 for reads, ignore
> writes" -- the point is that there is no single default
> behaviour that works for every device and architecture).
> Forcing the device model author to explicitly write the
> code means they have to think about what the behaviour
> they want is.

Yes, either way can prevent the NULL dereferencing. So it's time to
revisit this topic :)

>
> If we do choose to support allowing MemoryRegionOps structs
> to leave all the pointers NULL, we should document that,
> including what the default behaviour is.

Regards,
Bin