softmmu/memory.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
{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>
---
softmmu/memory.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4d..b97ffd4ba7 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1426,12 +1426,14 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
mr->ops->impl.max_access_size,
memory_region_read_accessor,
mr, attrs);
- } else {
+ } else if (mr->ops->read_with_attrs) {
return access_with_adjusted_size(addr, pval, size,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
memory_region_read_with_attrs_accessor,
mr, attrs);
+ } else {
+ return MEMTX_ERROR;
}
}
@@ -1506,13 +1508,15 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
mr->ops->impl.max_access_size,
memory_region_write_accessor, mr,
attrs);
- } else {
+ } else if (mr->ops->write_with_attrs) {
return
access_with_adjusted_size(addr, &data, size,
mr->ops->impl.min_access_size,
mr->ops->impl.max_access_size,
memory_region_write_with_attrs_accessor,
mr, attrs);
+ } else {
+ return MEMTX_ERROR;
}
}
--
2.25.1
Cc'ing PJP for https://www.mail-archive.com/qemu-devel@nongnu.org/msg730311.html
On Sun, Sep 5, 2021 at 5:41 PM 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>
> ---
>
> softmmu/memory.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4d..b97ffd4ba7 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1426,12 +1426,14 @@ static MemTxResult memory_region_dispatch_read1(MemoryRegion *mr,
> mr->ops->impl.max_access_size,
> memory_region_read_accessor,
> mr, attrs);
> - } else {
> + } else if (mr->ops->read_with_attrs) {
> return access_with_adjusted_size(addr, pval, size,
> mr->ops->impl.min_access_size,
> mr->ops->impl.max_access_size,
> memory_region_read_with_attrs_accessor,
> mr, attrs);
> + } else {
> + return MEMTX_ERROR;
> }
> }
>
> @@ -1506,13 +1508,15 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
> mr->ops->impl.max_access_size,
> memory_region_write_accessor, mr,
> attrs);
> - } else {
> + } else if (mr->ops->write_with_attrs) {
> return
> access_with_adjusted_size(addr, &data, size,
> mr->ops->impl.min_access_size,
> mr->ops->impl.max_access_size,
> memory_region_write_with_attrs_accessor,
> mr, attrs);
> + } else {
> + return MEMTX_ERROR;
> }
> }
>
> --
> 2.25.1
>
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. 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?
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.
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.
thanks
-- PMM
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
On Sun, 5 Sept 2021 at 17:49, Bin Meng <bmeng.cn@gmail.com> wrote:
>
> 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.
I mean that before commit 62a0db942dec leaving the pointers all
NULL was not allowed, and after that commit leaving the pointers all
NULL was still not allowed. It's been a requirement that you
provide at least one function pointer for read and one for
write ever since the MemoryRegion APIs were introduced in 2012.
You're proposing an API change; it might be a good one, but it
isn't a 'Fixes' to anything.
-- PMM
On Mon, Sep 6, 2021 at 12:54 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 5 Sept 2021 at 17:49, Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > 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.
>
> I mean that before commit 62a0db942dec leaving the pointers all
> NULL was not allowed, and after that commit leaving the pointers all
> NULL was still not allowed. It's been a requirement that you
> provide at least one function pointer for read and one for
> write ever since the MemoryRegion APIs were introduced in 2012.
> You're proposing an API change; it might be a good one, but it
> isn't a 'Fixes' to anything.
Where is the requirement documented? I don't see anything mentioned in
docs/devel/memory.rst
If it's a requirement since 2012, then I agree "Fixes" can be dropped.
But a doc fix should be made to document the "requirement".
Regards,
Bin
On Sun, 5 Sept 2021 at 18:07, Bin Meng <bmeng.cn@gmail.com> wrote: > > On Mon, Sep 6, 2021 at 12:54 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > I mean that before commit 62a0db942dec leaving the pointers all > > NULL was not allowed, and after that commit leaving the pointers all > > NULL was still not allowed. It's been a requirement that you > > provide at least one function pointer for read and one for > > write ever since the MemoryRegion APIs were introduced in 2012. > > You're proposing an API change; it might be a good one, but it > > isn't a 'Fixes' to anything. > > Where is the requirement documented? I don't see anything mentioned in > docs/devel/memory.rst It's not documented, but lots of fiddly details of QEMU functions aren't documented... > If it's a requirement since 2012, then I agree "Fixes" can be dropped. > But a doc fix should be made to document the "requirement". Agreed. -- PMM
On 05/09/21 18:29, Peter Maydell wrote: > 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 I agree, and Prasad had even posted a patch for that, but it failed some qtests and I ended up never checking why that was. Paolo
© 2016 - 2024 Red Hat, Inc.