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

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

[Bin Meng]

{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

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

[Philippe Mathieu-Daudé]

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
>

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

[Peter Maydell]

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

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

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

[Peter Maydell]

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

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

[Bin Meng]

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

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

[Peter Maydell]

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

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

[Paolo Bonzini]

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