[PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Michael S. Tsirkin]

Memory API documentation documents valid .min_access_size and .max_access_size
fields and explains that any access outside these boundaries is blocked.

This is what devices seem to assume.

However this is not what the implementation does: it simply
ignores the boundaries unless there's an "accepts" callback.

Naturally, this breaks a bunch of devices.

Revert to the documented behaviour.

Devices that want to allow any access can just drop the valid field,
or add the impl field to have accesses converted to appropriate
length.

Cc: qemu-stable@nongnu.org
Reviewed-by: Richard Henderson <rth@twiddle.net>
Fixes: CVE-2020-13754
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 memory.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/memory.c b/memory.c
index 91ceaf9fcf..3e9388fb74 100644
--- a/memory.c
+++ b/memory.c
@@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
                                 bool is_write,
                                 MemTxAttrs attrs)
 {
-    int access_size_min, access_size_max;
-    int access_size, i;
+    if (mr->ops->valid.accepts
+        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
+        return false;
+    }
 
     if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
         return false;
     }
 
-    if (!mr->ops->valid.accepts) {
+    /* Treat zero as compatibility all valid */
+    if (!mr->ops->valid.max_access_size) {
         return true;
     }
 
-    access_size_min = mr->ops->valid.min_access_size;
-    if (!mr->ops->valid.min_access_size) {
-        access_size_min = 1;
+    if (size > mr->ops->valid.max_access_size
+        || size < mr->ops->valid.min_access_size) {
+        return false;
     }
-
-    access_size_max = mr->ops->valid.max_access_size;
-    if (!mr->ops->valid.max_access_size) {
-        access_size_max = 4;
-    }
-
-    access_size = MAX(MIN(size, access_size_max), access_size_min);
-    for (i = 0; i < size; i += access_size) {
-        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
-                                    is_write, attrs)) {
-            return false;
-        }
-    }
-
     return true;
 }
 
-- 
MST

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Michael S. Tsirkin]

On Wed, Jun 10, 2020 at 09:47:52AM -0400, Michael S. Tsirkin wrote:
> Memory API documentation documents valid .min_access_size and .max_access_size
> fields and explains that any access outside these boundaries is blocked.
> 
> This is what devices seem to assume.
> 
> However this is not what the implementation does: it simply
> ignores the boundaries unless there's an "accepts" callback.
> 
> Naturally, this breaks a bunch of devices.
> 
> Revert to the documented behaviour.
> 
> Devices that want to allow any access can just drop the valid field,
> or add the impl field to have accesses converted to appropriate
> length.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Richard Henderson <rth@twiddle.net>

Actually, I rechecked and couldn't find this tag in my inbox.
So maybe this should have been:
Cc: Richard Henderson <rth@twiddle.net>
Or maybe I lost that email.

Richard could you ack this explicitly pls to avoid confusion?

> Fixes: CVE-2020-13754
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  memory.c | 29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 91ceaf9fcf..3e9388fb74 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
>                                  bool is_write,
>                                  MemTxAttrs attrs)
>  {
> -    int access_size_min, access_size_max;
> -    int access_size, i;
> +    if (mr->ops->valid.accepts
> +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> +        return false;
> +    }
>  
>      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
>          return false;
>      }
>  
> -    if (!mr->ops->valid.accepts) {
> +    /* Treat zero as compatibility all valid */
> +    if (!mr->ops->valid.max_access_size) {
>          return true;
>      }
>  
> -    access_size_min = mr->ops->valid.min_access_size;
> -    if (!mr->ops->valid.min_access_size) {
> -        access_size_min = 1;
> +    if (size > mr->ops->valid.max_access_size
> +        || size < mr->ops->valid.min_access_size) {
> +        return false;
>      }
> -
> -    access_size_max = mr->ops->valid.max_access_size;
> -    if (!mr->ops->valid.max_access_size) {
> -        access_size_max = 4;
> -    }
> -
> -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> -    for (i = 0; i < size; i += access_size) {
> -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> -                                    is_write, attrs)) {
> -            return false;
> -        }
> -    }
> -
>      return true;
>  }
>  
> -- 
> MST

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Paolo Bonzini]

On 10/06/20 15:47, Michael S. Tsirkin wrote:
> Memory API documentation documents valid .min_access_size and .max_access_size
> fields and explains that any access outside these boundaries is blocked.
> 
> This is what devices seem to assume.
> 
> However this is not what the implementation does: it simply
> ignores the boundaries unless there's an "accepts" callback.
> 
> Naturally, this breaks a bunch of devices.
> 
> Revert to the documented behaviour.
> 
> Devices that want to allow any access can just drop the valid field,
> or add the impl field to have accesses converted to appropriate
> length.
> 
> Cc: qemu-stable@nongnu.org
> Reviewed-by: Richard Henderson <rth@twiddle.net>
> Fixes: CVE-2020-13754
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  memory.c | 29 +++++++++--------------------
>  1 file changed, 9 insertions(+), 20 deletions(-)
> 
> diff --git a/memory.c b/memory.c
> index 91ceaf9fcf..3e9388fb74 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
>                                  bool is_write,
>                                  MemTxAttrs attrs)
>  {
> -    int access_size_min, access_size_max;
> -    int access_size, i;
> +    if (mr->ops->valid.accepts
> +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> +        return false;
> +    }
>  
>      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
>          return false;
>      }
>  
> -    if (!mr->ops->valid.accepts) {
> +    /* Treat zero as compatibility all valid */
> +    if (!mr->ops->valid.max_access_size) {
>          return true;
>      }
>  
> -    access_size_min = mr->ops->valid.min_access_size;
> -    if (!mr->ops->valid.min_access_size) {
> -        access_size_min = 1;
> +    if (size > mr->ops->valid.max_access_size
> +        || size < mr->ops->valid.min_access_size) {
> +        return false;
>      }
> -
> -    access_size_max = mr->ops->valid.max_access_size;
> -    if (!mr->ops->valid.max_access_size) {
> -        access_size_max = 4;
> -    }
> -
> -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> -    for (i = 0; i < size; i += access_size) {
> -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> -                                    is_write, attrs)) {
> -            return false;
> -        }
> -    }
> -
>      return true;
>  }
>  
> 

Queued, thanks.

Paolo

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Alistair Francis]

On Wed, Aug 26, 2020 at 11:26 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Hi all,
>
> Sorry for the duplicate reply, my first one was rejected by a mailing
> list administrator for being too long so I resent it with the error logs
> as a link instead of inline.
>
> On Wed, Jun 10, 2020 at 09:47:49AM -0400, Michael S. Tsirkin wrote:
> > Memory API documentation documents valid .min_access_size and .max_access_size
> > fields and explains that any access outside these boundaries is blocked.
> >
> > This is what devices seem to assume.
> >
> > However this is not what the implementation does: it simply
> > ignores the boundaries unless there's an "accepts" callback.
> >
> > Naturally, this breaks a bunch of devices.
> >
> > Revert to the documented behaviour.
> >
> > Devices that want to allow any access can just drop the valid field,
> > or add the impl field to have accesses converted to appropriate
> > length.
> >
> > Cc: qemu-stable@nongnu.org
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > Fixes: CVE-2020-13754
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> > Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  memory.c | 29 +++++++++--------------------
> >  1 file changed, 9 insertions(+), 20 deletions(-)
> >
> > diff --git a/memory.c b/memory.c
> > index 91ceaf9fcf..3e9388fb74 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
> >                                  bool is_write,
> >                                  MemTxAttrs attrs)
> >  {
> > -    int access_size_min, access_size_max;
> > -    int access_size, i;
> > +    if (mr->ops->valid.accepts
> > +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> > +        return false;
> > +    }
> >
> >      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> >          return false;
> >      }
> >
> > -    if (!mr->ops->valid.accepts) {
> > +    /* Treat zero as compatibility all valid */
> > +    if (!mr->ops->valid.max_access_size) {
> >          return true;
> >      }
> >
> > -    access_size_min = mr->ops->valid.min_access_size;
> > -    if (!mr->ops->valid.min_access_size) {
> > -        access_size_min = 1;
> > +    if (size > mr->ops->valid.max_access_size
> > +        || size < mr->ops->valid.min_access_size) {
> > +        return false;
> >      }
> > -
> > -    access_size_max = mr->ops->valid.max_access_size;
> > -    if (!mr->ops->valid.max_access_size) {
> > -        access_size_max = 4;
> > -    }
> > -
> > -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> > -    for (i = 0; i < size; i += access_size) {
> > -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> > -                                    is_write, attrs)) {
> > -            return false;
> > -        }
> > -    }
> > -
> >      return true;
> >  }
> >
> > --
> > MST
> >
> >
>
> I just ran into a regression with booting RISC-V kernels due to this
> commit. I can reproduce it with QEMU 5.1.0 and latest tip of tree
> (25f6dc28a3a8dd231c2c092a0e65bd796353c769 at the time of initially
> writing this).
>
> The error message, commands, and bisect logs are available here:
>
> https://gist.githubusercontent.com/nathanchance/c106dd22ec0c0e00f6a25daba106a1b9/raw/d929f2fff6da9126ded156affb0f19f359e9f693/qemu-5.1.0-issue-terminal-log.txt

From what I can tell the problem happens when you try to reboot the board right?

You might want to try changing this line from 4 to 8:
https://github.com/qemu/qemu/blob/master/hw/riscv/sifive_test.c#L63

>
> I have attached the rootfs and kernel image used for these tests. If for
> some reason there is a problem receiving them, the kernel is just an
> arch/riscv/configs/defconfig kernel at Linux 5.9-rc2 and the rootfs is
> available here:
>
> https://github.com/ClangBuiltLinux/boot-utils/blob/3b21a5b71451742866349ba4f18638c5a754e660/images/riscv/rootfs.cpio.zst
>
> Please let me know if I can provide any follow up information or if I am
> doing something wrong.

Thanks for providing so much information and doing a bisect.

Alistair

>
> Cheers,
> Nathan

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Nathan Chancellor]

On Thu, Aug 27, 2020 at 08:53:30AM -0700, Alistair Francis wrote:
> On Wed, Aug 26, 2020 at 11:26 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > Hi all,
> >
> > Sorry for the duplicate reply, my first one was rejected by a mailing
> > list administrator for being too long so I resent it with the error logs
> > as a link instead of inline.
> >
> > On Wed, Jun 10, 2020 at 09:47:49AM -0400, Michael S. Tsirkin wrote:
> > > Memory API documentation documents valid .min_access_size and .max_access_size
> > > fields and explains that any access outside these boundaries is blocked.
> > >
> > > This is what devices seem to assume.
> > >
> > > However this is not what the implementation does: it simply
> > > ignores the boundaries unless there's an "accepts" callback.
> > >
> > > Naturally, this breaks a bunch of devices.
> > >
> > > Revert to the documented behaviour.
> > >
> > > Devices that want to allow any access can just drop the valid field,
> > > or add the impl field to have accesses converted to appropriate
> > > length.
> > >
> > > Cc: qemu-stable@nongnu.org
> > > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > > Fixes: CVE-2020-13754
> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> > > Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  memory.c | 29 +++++++++--------------------
> > >  1 file changed, 9 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/memory.c b/memory.c
> > > index 91ceaf9fcf..3e9388fb74 100644
> > > --- a/memory.c
> > > +++ b/memory.c
> > > @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
> > >                                  bool is_write,
> > >                                  MemTxAttrs attrs)
> > >  {
> > > -    int access_size_min, access_size_max;
> > > -    int access_size, i;
> > > +    if (mr->ops->valid.accepts
> > > +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> > > +        return false;
> > > +    }
> > >
> > >      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> > >          return false;
> > >      }
> > >
> > > -    if (!mr->ops->valid.accepts) {
> > > +    /* Treat zero as compatibility all valid */
> > > +    if (!mr->ops->valid.max_access_size) {
> > >          return true;
> > >      }
> > >
> > > -    access_size_min = mr->ops->valid.min_access_size;
> > > -    if (!mr->ops->valid.min_access_size) {
> > > -        access_size_min = 1;
> > > +    if (size > mr->ops->valid.max_access_size
> > > +        || size < mr->ops->valid.min_access_size) {
> > > +        return false;
> > >      }
> > > -
> > > -    access_size_max = mr->ops->valid.max_access_size;
> > > -    if (!mr->ops->valid.max_access_size) {
> > > -        access_size_max = 4;
> > > -    }
> > > -
> > > -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> > > -    for (i = 0; i < size; i += access_size) {
> > > -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> > > -                                    is_write, attrs)) {
> > > -            return false;
> > > -        }
> > > -    }
> > > -
> > >      return true;
> > >  }
> > >
> > > --
> > > MST
> > >
> > >
> >
> > I just ran into a regression with booting RISC-V kernels due to this
> > commit. I can reproduce it with QEMU 5.1.0 and latest tip of tree
> > (25f6dc28a3a8dd231c2c092a0e65bd796353c769 at the time of initially
> > writing this).
> >
> > The error message, commands, and bisect logs are available here:
> >
> > https://gist.githubusercontent.com/nathanchance/c106dd22ec0c0e00f6a25daba106a1b9/raw/d929f2fff6da9126ded156affb0f19f359e9f693/qemu-5.1.0-issue-terminal-log.txt
> 
> From what I can tell the problem happens when you try to reboot the board right?

Yes, sorry, should have made that clear. All the rootfs does is print
the version string and then runs 'poweroff' (not 'reboot'):

https://github.com/ClangBuiltLinux/boot-utils/blob/master/buildroot/overlay-poweroff/etc/init.d/S50yolo

> You might want to try changing this line from 4 to 8:
> https://github.com/qemu/qemu/blob/master/hw/riscv/sifive_test.c#L63

Unfortunately, that did not work. For the record, I tried changing that
field in all drivers in hw/riscv:

$ sed -i 's/ \.max_access_size = .*/ \.max_access_size = 8/' hw/riscv/* &&
./configure &&
make -j"$(nproc)"

> >
> > I have attached the rootfs and kernel image used for these tests. If for
> > some reason there is a problem receiving them, the kernel is just an
> > arch/riscv/configs/defconfig kernel at Linux 5.9-rc2 and the rootfs is
> > available here:
> >
> > https://github.com/ClangBuiltLinux/boot-utils/blob/3b21a5b71451742866349ba4f18638c5a754e660/images/riscv/rootfs.cpio.zst
> >
> > Please let me know if I can provide any follow up information or if I am
> > doing something wrong.
> 
> Thanks for providing so much information and doing a bisect.
> 
> Alistair
> 
> >
> > Cheers,
> > Nathan

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Michael S. Tsirkin]

On Wed, Aug 26, 2020 at 10:32:16PM -0700, Nathan Chancellor wrote:
> Hi all,
> 
> Sorry for the duplicate reply, my first one was rejected by a mailing
> list administrator for being too long so I resent it with the error logs
> as a link instead of inline.
> 
> On Wed, Jun 10, 2020 at 09:47:49AM -0400, Michael S. Tsirkin wrote:
> > Memory API documentation documents valid .min_access_size and .max_access_size
> > fields and explains that any access outside these boundaries is blocked.
> > 
> > This is what devices seem to assume.
> > 
> > However this is not what the implementation does: it simply
> > ignores the boundaries unless there's an "accepts" callback.
> > 
> > Naturally, this breaks a bunch of devices.
> > 
> > Revert to the documented behaviour.
> > 
> > Devices that want to allow any access can just drop the valid field,
> > or add the impl field to have accesses converted to appropriate
> > length.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > Fixes: CVE-2020-13754
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> > Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  memory.c | 29 +++++++++--------------------
> >  1 file changed, 9 insertions(+), 20 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 91ceaf9fcf..3e9388fb74 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
> >                                  bool is_write,
> >                                  MemTxAttrs attrs)
> >  {
> > -    int access_size_min, access_size_max;
> > -    int access_size, i;
> > +    if (mr->ops->valid.accepts
> > +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> > +        return false;
> > +    }
> >  
> >      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> >          return false;
> >      }
> >  
> > -    if (!mr->ops->valid.accepts) {
> > +    /* Treat zero as compatibility all valid */
> > +    if (!mr->ops->valid.max_access_size) {
> >          return true;
> >      }
> >  
> > -    access_size_min = mr->ops->valid.min_access_size;
> > -    if (!mr->ops->valid.min_access_size) {
> > -        access_size_min = 1;
> > +    if (size > mr->ops->valid.max_access_size
> > +        || size < mr->ops->valid.min_access_size) {
> > +        return false;
> >      }
> > -
> > -    access_size_max = mr->ops->valid.max_access_size;
> > -    if (!mr->ops->valid.max_access_size) {
> > -        access_size_max = 4;
> > -    }
> > -
> > -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> > -    for (i = 0; i < size; i += access_size) {
> > -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> > -                                    is_write, attrs)) {
> > -            return false;
> > -        }
> > -    }
> > -
> >      return true;
> >  }
> >  
> > -- 
> > MST
> > 
> > 
> 
> I just ran into a regression with booting RISC-V kernels due to this
> commit. I can reproduce it with QEMU 5.1.0 and latest tip of tree
> (25f6dc28a3a8dd231c2c092a0e65bd796353c769 at the time of initially
> writing this).
> 
> The error message, commands, and bisect logs are available here:
> 
> https://gist.githubusercontent.com/nathanchance/c106dd22ec0c0e00f6a25daba106a1b9/raw/d929f2fff6da9126ded156affb0f19f359e9f693/qemu-5.1.0-issue-terminal-log.txt
> 
> I have attached the rootfs and kernel image used for these tests. If for
> some reason there is a problem receiving them, the kernel is just an
> arch/riscv/configs/defconfig kernel at Linux 5.9-rc2 and the rootfs is
> available here:
> 
> https://github.com/ClangBuiltLinux/boot-utils/blob/3b21a5b71451742866349ba4f18638c5a754e660/images/riscv/rootfs.cpio.zst
> 
> Please let me know if I can provide any follow up information or if I am
> doing something wrong.
> 
> Cheers,
> Nathan


The following patch was proposed to fix the issue:

-----------------------------------------------------------
 hw/display/tcx.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/hw/display/tcx.c b/hw/display/tcx.c
index 1fb45b1aab8..96c6898b149 100644
--- a/hw/display/tcx.c
+++ b/hw/display/tcx.c
@@ -548,20 +548,28 @@ static const MemoryRegionOps tcx_stip_ops = {
     .read = tcx_stip_readl,
     .write = tcx_stip_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
 };
 
 static const MemoryRegionOps tcx_rstip_ops = {
     .read = tcx_stip_readl,
     .write = tcx_rstip_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
 };
 
 static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
@@ -650,10 +658,14 @@ static const MemoryRegionOps tcx_rblit_ops = {
     .read = tcx_blit_readl,
     .write = tcx_rblit_writel,
     .endianness = DEVICE_NATIVE_ENDIAN,
-    .valid = {
+    .impl = {
         .min_access_size = 4,
         .max_access_size = 4,
     },
+    .valid = {
+        .min_access_size = 4,
+        .max_access_size = 8,
+    },
 };
 
 static void tcx_invalidate_cursor_position(TCXState *s)


-----------------------------------------------------------

does this fix the issue for you?


> -- 
> 2.26.2
> 
> -- 
> You received this bug notification because you are subscribed to the bug
> report.
> https://bugs.launchpad.net/bugs/1892540
> 
> Title:
>   qemu can no longer boot NetBSD/sparc
> 
> Status in QEMU:
>   New
> 
> Bug description:
>   Booting NetBSD/sparc in qemu no longer works.  It broke between qemu
>   version 5.0.0 and 5.1.0, and a bisection identified the following as
>   the offending commit:
> 
>     [5d971f9e672507210e77d020d89e0e89165c8fc9] memory: Revert "memory:
>   accept mismatching sizes in memory_region_access_valid"
> 
>   It's still broken as of 7fd51e68c34fcefdb4d6fd646ed3346f780f89f4.
> 
>   To reproduce, run
> 
>     wget http://ftp.netbsd.org/pub/NetBSD/NetBSD-9.0/images/NetBSD-9.0-sparc.iso
>     qemu-system-sparc -nographic -cdrom NetBSD-9.0-sparc.iso -boot d
> 
>   The expected behavior is that the guest boots to the prompt
> 
>     Installation medium to load the additional utilities from:
> 
>   The observed behavior is a panic:
> 
>     [   1.0000050] system[0]: trap 0x29: pc=0xf0046b14 sfsr=0xb6 sfva=0x54000000
>     [   1.0000050] cpu0: data fault: pc=0xf0046b14 addr=0x54000000 sfsr=0xb6<PERR=0x0,LVL=0x0,AT=0x5,FT=0x5,FAV,OW>
>     [   1.0000050] panic: kernel fault
>     [   1.0000050] halted
> 
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/qemu/+bug/1892540/+subscriptions

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Nathan Chancellor]

On Sun, Aug 30, 2020 at 02:20:38AM -0400, Michael S. Tsirkin wrote:
> On Wed, Aug 26, 2020 at 10:32:16PM -0700, Nathan Chancellor wrote:
> > Hi all,
> > 
> > Sorry for the duplicate reply, my first one was rejected by a mailing
> > list administrator for being too long so I resent it with the error logs
> > as a link instead of inline.
> > 
> > On Wed, Jun 10, 2020 at 09:47:49AM -0400, Michael S. Tsirkin wrote:
> > > Memory API documentation documents valid .min_access_size and .max_access_size
> > > fields and explains that any access outside these boundaries is blocked.
> > > 
> > > This is what devices seem to assume.
> > > 
> > > However this is not what the implementation does: it simply
> > > ignores the boundaries unless there's an "accepts" callback.
> > > 
> > > Naturally, this breaks a bunch of devices.
> > > 
> > > Revert to the documented behaviour.
> > > 
> > > Devices that want to allow any access can just drop the valid field,
> > > or add the impl field to have accesses converted to appropriate
> > > length.
> > > 
> > > Cc: qemu-stable@nongnu.org
> > > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > > Fixes: CVE-2020-13754
> > > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> > > Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > >  memory.c | 29 +++++++++--------------------
> > >  1 file changed, 9 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/memory.c b/memory.c
> > > index 91ceaf9fcf..3e9388fb74 100644
> > > --- a/memory.c
> > > +++ b/memory.c
> > > @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
> > >                                  bool is_write,
> > >                                  MemTxAttrs attrs)
> > >  {
> > > -    int access_size_min, access_size_max;
> > > -    int access_size, i;
> > > +    if (mr->ops->valid.accepts
> > > +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> > > +        return false;
> > > +    }
> > >  
> > >      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> > >          return false;
> > >      }
> > >  
> > > -    if (!mr->ops->valid.accepts) {
> > > +    /* Treat zero as compatibility all valid */
> > > +    if (!mr->ops->valid.max_access_size) {
> > >          return true;
> > >      }
> > >  
> > > -    access_size_min = mr->ops->valid.min_access_size;
> > > -    if (!mr->ops->valid.min_access_size) {
> > > -        access_size_min = 1;
> > > +    if (size > mr->ops->valid.max_access_size
> > > +        || size < mr->ops->valid.min_access_size) {
> > > +        return false;
> > >      }
> > > -
> > > -    access_size_max = mr->ops->valid.max_access_size;
> > > -    if (!mr->ops->valid.max_access_size) {
> > > -        access_size_max = 4;
> > > -    }
> > > -
> > > -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> > > -    for (i = 0; i < size; i += access_size) {
> > > -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> > > -                                    is_write, attrs)) {
> > > -            return false;
> > > -        }
> > > -    }
> > > -
> > >      return true;
> > >  }
> > >  
> > > -- 
> > > MST
> > > 
> > > 
> > 
> > I just ran into a regression with booting RISC-V kernels due to this
> > commit. I can reproduce it with QEMU 5.1.0 and latest tip of tree
> > (25f6dc28a3a8dd231c2c092a0e65bd796353c769 at the time of initially
> > writing this).
> > 
> > The error message, commands, and bisect logs are available here:
> > 
> > https://gist.githubusercontent.com/nathanchance/c106dd22ec0c0e00f6a25daba106a1b9/raw/d929f2fff6da9126ded156affb0f19f359e9f693/qemu-5.1.0-issue-terminal-log.txt
> > 
> > I have attached the rootfs and kernel image used for these tests. If for
> > some reason there is a problem receiving them, the kernel is just an
> > arch/riscv/configs/defconfig kernel at Linux 5.9-rc2 and the rootfs is
> > available here:
> > 
> > https://github.com/ClangBuiltLinux/boot-utils/blob/3b21a5b71451742866349ba4f18638c5a754e660/images/riscv/rootfs.cpio.zst
> > 
> > Please let me know if I can provide any follow up information or if I am
> > doing something wrong.
> > 
> > Cheers,
> > Nathan
> 
> 
> The following patch was proposed to fix the issue:
> 
> -----------------------------------------------------------
>  hw/display/tcx.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/display/tcx.c b/hw/display/tcx.c
> index 1fb45b1aab8..96c6898b149 100644
> --- a/hw/display/tcx.c
> +++ b/hw/display/tcx.c
> @@ -548,20 +548,28 @@ static const MemoryRegionOps tcx_stip_ops = {
>      .read = tcx_stip_readl,
>      .write = tcx_stip_writel,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> +    .impl = {
>          .min_access_size = 4,
>          .max_access_size = 4,
>      },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
>  };
>  
>  static const MemoryRegionOps tcx_rstip_ops = {
>      .read = tcx_stip_readl,
>      .write = tcx_rstip_writel,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> +    .impl = {
>          .min_access_size = 4,
>          .max_access_size = 4,
>      },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
>  };
>  
>  static uint64_t tcx_blit_readl(void *opaque, hwaddr addr,
> @@ -650,10 +658,14 @@ static const MemoryRegionOps tcx_rblit_ops = {
>      .read = tcx_blit_readl,
>      .write = tcx_rblit_writel,
>      .endianness = DEVICE_NATIVE_ENDIAN,
> -    .valid = {
> +    .impl = {
>          .min_access_size = 4,
>          .max_access_size = 4,
>      },
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 8,
> +    },
>  };
>  
>  static void tcx_invalidate_cursor_position(TCXState *s)
> 
> 
> -----------------------------------------------------------
> 
> does this fix the issue for you?

Unfortunately, it does not. I applied it on top of latest
git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
reproduce my failure. Is it possible that type of fix is needed
in a RISC-V specific driver?

Would you like me to comment on the Launchpad bug as well?

Cheers,
Nathan

> > -- 
> > 2.26.2
> > 
> > -- 
> > You received this bug notification because you are subscribed to the bug
> > report.
> > https://bugs.launchpad.net/bugs/1892540
> > 
> > Title:
> >   qemu can no longer boot NetBSD/sparc
> > 
> > Status in QEMU:
> >   New
> > 
> > Bug description:
> >   Booting NetBSD/sparc in qemu no longer works.  It broke between qemu
> >   version 5.0.0 and 5.1.0, and a bisection identified the following as
> >   the offending commit:
> > 
> >     [5d971f9e672507210e77d020d89e0e89165c8fc9] memory: Revert "memory:
> >   accept mismatching sizes in memory_region_access_valid"
> > 
> >   It's still broken as of 7fd51e68c34fcefdb4d6fd646ed3346f780f89f4.
> > 
> >   To reproduce, run
> > 
> >     wget http://ftp.netbsd.org/pub/NetBSD/NetBSD-9.0/images/NetBSD-9.0-sparc.iso
> >     qemu-system-sparc -nographic -cdrom NetBSD-9.0-sparc.iso -boot d
> > 
> >   The expected behavior is that the guest boots to the prompt
> > 
> >     Installation medium to load the additional utilities from:
> > 
> >   The observed behavior is a panic:
> > 
> >     [   1.0000050] system[0]: trap 0x29: pc=0xf0046b14 sfsr=0xb6 sfva=0x54000000
> >     [   1.0000050] cpu0: data fault: pc=0xf0046b14 addr=0x54000000 sfsr=0xb6<PERR=0x0,LVL=0x0,AT=0x5,FT=0x5,FAV,OW>
> >     [   1.0000050] panic: kernel fault
> >     [   1.0000050] halted
> > 
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/qemu/+bug/1892540/+subscriptions
> 
> 
> 

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Mark Cave-Ayland]

On 30/08/2020 07:49, Nathan Chancellor wrote:

> Unfortunately, it does not. I applied it on top of latest
> git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
> reproduce my failure. Is it possible that type of fix is needed
> in a RISC-V specific driver?
> 
> Would you like me to comment on the Launchpad bug as well?

Hi Nathan,

I came up with a quick patch to enable some logging to catch memory accesses being
refused for a similar bug report at
https://bugs.launchpad.net/qemu/+bug/1886318/comments/13.

Can you apply the same change to your tree and report any messages on stderr as you
do your board reboot test?


ATB,

Mark.

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Nathan Chancellor]

On Sun, Aug 30, 2020 at 08:24:15AM +0100, Mark Cave-Ayland wrote:
> On 30/08/2020 07:49, Nathan Chancellor wrote:
> 
> > Unfortunately, it does not. I applied it on top of latest
> > git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
> > reproduce my failure. Is it possible that type of fix is needed
> > in a RISC-V specific driver?
> > 
> > Would you like me to comment on the Launchpad bug as well?
> 
> Hi Nathan,
> 
> I came up with a quick patch to enable some logging to catch memory accesses being
> refused for a similar bug report at
> https://bugs.launchpad.net/qemu/+bug/1886318/comments/13.
> 
> Can you apply the same change to your tree and report any messages on stderr as you
> do your board reboot test?
> 
> 
> ATB,
> 
> Mark.

Thanks Mark, that helped.

...
[    3.807738] reboot: Power down
invalid size: riscv.sifive.test addr 0 size: 2
sbi_trap_error: hart0: trap handler failed (error -2)
sbi_trap_error: hart0: mcause=0x0000000000000007 mtval=0x0000000000100000
sbi_trap_error: hart0: mepc=0x000000008000d4cc mstatus=0x0000000000001822
sbi_trap_error: hart0: ra=0x000000008000999e sp=0x0000000080015c78
sbi_trap_error: hart0: gp=0xffffffe000e765d0 tp=0xffffffe0081c0000
sbi_trap_error: hart0: s0=0x0000000080015c88 s1=0x0000000000000040
sbi_trap_error: hart0: a0=0x0000000000000000 a1=0x0000000080004024
sbi_trap_error: hart0: a2=0x0000000080004024 a3=0x0000000080004024
sbi_trap_error: hart0: a4=0x0000000000100000 a5=0x0000000000005555
sbi_trap_error: hart0: a6=0x0000000000004024 a7=0x0000000080011158
sbi_trap_error: hart0: s2=0x0000000000000000 s3=0x0000000080016000
sbi_trap_error: hart0: s4=0x0000000000000000 s5=0x0000000000000000
sbi_trap_error: hart0: s6=0x0000000000000001 s7=0x0000000000000000
sbi_trap_error: hart0: s8=0x0000000000000000 s9=0x0000000000000000
sbi_trap_error: hart0: s10=0x0000000000000000 s11=0x0000000000000008
sbi_trap_error: hart0: t0=0x0000000000000000 t1=0x0000000000000000
sbi_trap_error: hart0: t2=0x0000000000000000 t3=0x0000000000000000
sbi_trap_error: hart0: t4=0x0000000000000000 t5=0x0000000000000000
sbi_trap_error: hart0: t6=0x0000000000000000

With this diff, I can successfully shut down the board. No idea if that
is valid or not though.

Cheers,
Nathan

============================================================

diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
index 0c78fb2c93..8c70dd69df 100644
--- a/hw/riscv/sifive_test.c
+++ b/hw/riscv/sifive_test.c
@@ -59,7 +59,7 @@ static const MemoryRegionOps sifive_test_ops = {
     .write = sifive_test_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .valid = {
-        .min_access_size = 4,
+        .min_access_size = 2,
         .max_access_size = 4
     }
 };

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Mark Cave-Ayland]

On 30/08/2020 08:43, Nathan Chancellor wrote:

> On Sun, Aug 30, 2020 at 08:24:15AM +0100, Mark Cave-Ayland wrote:
>> On 30/08/2020 07:49, Nathan Chancellor wrote:
>>
>>> Unfortunately, it does not. I applied it on top of latest
>>> git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
>>> reproduce my failure. Is it possible that type of fix is needed
>>> in a RISC-V specific driver?
>>>
>>> Would you like me to comment on the Launchpad bug as well?
>>
>> Hi Nathan,
>>
>> I came up with a quick patch to enable some logging to catch memory accesses being
>> refused for a similar bug report at
>> https://bugs.launchpad.net/qemu/+bug/1886318/comments/13.
>>
>> Can you apply the same change to your tree and report any messages on stderr as you
>> do your board reboot test?
>>
>>
>> ATB,
>>
>> Mark.
> 
> Thanks Mark, that helped.
> 
> ...
> [    3.807738] reboot: Power down
> invalid size: riscv.sifive.test addr 0 size: 2
> sbi_trap_error: hart0: trap handler failed (error -2)
> sbi_trap_error: hart0: mcause=0x0000000000000007 mtval=0x0000000000100000
> sbi_trap_error: hart0: mepc=0x000000008000d4cc mstatus=0x0000000000001822
> sbi_trap_error: hart0: ra=0x000000008000999e sp=0x0000000080015c78
> sbi_trap_error: hart0: gp=0xffffffe000e765d0 tp=0xffffffe0081c0000
> sbi_trap_error: hart0: s0=0x0000000080015c88 s1=0x0000000000000040
> sbi_trap_error: hart0: a0=0x0000000000000000 a1=0x0000000080004024
> sbi_trap_error: hart0: a2=0x0000000080004024 a3=0x0000000080004024
> sbi_trap_error: hart0: a4=0x0000000000100000 a5=0x0000000000005555
> sbi_trap_error: hart0: a6=0x0000000000004024 a7=0x0000000080011158
> sbi_trap_error: hart0: s2=0x0000000000000000 s3=0x0000000080016000
> sbi_trap_error: hart0: s4=0x0000000000000000 s5=0x0000000000000000
> sbi_trap_error: hart0: s6=0x0000000000000001 s7=0x0000000000000000
> sbi_trap_error: hart0: s8=0x0000000000000000 s9=0x0000000000000000
> sbi_trap_error: hart0: s10=0x0000000000000000 s11=0x0000000000000008
> sbi_trap_error: hart0: t0=0x0000000000000000 t1=0x0000000000000000
> sbi_trap_error: hart0: t2=0x0000000000000000 t3=0x0000000000000000
> sbi_trap_error: hart0: t4=0x0000000000000000 t5=0x0000000000000000
> sbi_trap_error: hart0: t6=0x0000000000000000
> 
> With this diff, I can successfully shut down the board. No idea if that
> is valid or not though.
> 
> Cheers,
> Nathan
> 
> ============================================================
> 
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 0c78fb2c93..8c70dd69df 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -59,7 +59,7 @@ static const MemoryRegionOps sifive_test_ops = {
>      .write = sifive_test_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
> -        .min_access_size = 4,
> +        .min_access_size = 2,
>          .max_access_size = 4
>      }
>  };

Okay - so according to the definition above, before your patch the sifive_test device
has a min and max access size of 4, i.e. all writes less than 32-bits are invalid.
With your patch you reduce the min access size to 2 which allows 16-bit writes and so
allows your shutdown test to succeed.

I'm afraid I'm not familiar enough with RISCV or the sifive_test device specification
to know whether the QEMU definition is correct and the access should be refused, or
whether the guest is using the wrong size when writing to the sifive_test device
register.


ATB,

Mark.

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Alistair Francis]

On Sun, Aug 30, 2020 at 12:43 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Sun, Aug 30, 2020 at 08:24:15AM +0100, Mark Cave-Ayland wrote:
> > On 30/08/2020 07:49, Nathan Chancellor wrote:
> >
> > > Unfortunately, it does not. I applied it on top of latest
> > > git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
> > > reproduce my failure. Is it possible that type of fix is needed
> > > in a RISC-V specific driver?
> > >
> > > Would you like me to comment on the Launchpad bug as well?
> >
> > Hi Nathan,
> >
> > I came up with a quick patch to enable some logging to catch memory accesses being
> > refused for a similar bug report at
> > https://bugs.launchpad.net/qemu/+bug/1886318/comments/13.
> >
> > Can you apply the same change to your tree and report any messages on stderr as you
> > do your board reboot test?
> >
> >
> > ATB,
> >
> > Mark.
>
> Thanks Mark, that helped.
>
> ...
> [    3.807738] reboot: Power down
> invalid size: riscv.sifive.test addr 0 size: 2
> sbi_trap_error: hart0: trap handler failed (error -2)
> sbi_trap_error: hart0: mcause=0x0000000000000007 mtval=0x0000000000100000
> sbi_trap_error: hart0: mepc=0x000000008000d4cc mstatus=0x0000000000001822
> sbi_trap_error: hart0: ra=0x000000008000999e sp=0x0000000080015c78
> sbi_trap_error: hart0: gp=0xffffffe000e765d0 tp=0xffffffe0081c0000
> sbi_trap_error: hart0: s0=0x0000000080015c88 s1=0x0000000000000040
> sbi_trap_error: hart0: a0=0x0000000000000000 a1=0x0000000080004024
> sbi_trap_error: hart0: a2=0x0000000080004024 a3=0x0000000080004024
> sbi_trap_error: hart0: a4=0x0000000000100000 a5=0x0000000000005555
> sbi_trap_error: hart0: a6=0x0000000000004024 a7=0x0000000080011158
> sbi_trap_error: hart0: s2=0x0000000000000000 s3=0x0000000080016000
> sbi_trap_error: hart0: s4=0x0000000000000000 s5=0x0000000000000000
> sbi_trap_error: hart0: s6=0x0000000000000001 s7=0x0000000000000000
> sbi_trap_error: hart0: s8=0x0000000000000000 s9=0x0000000000000000
> sbi_trap_error: hart0: s10=0x0000000000000000 s11=0x0000000000000008
> sbi_trap_error: hart0: t0=0x0000000000000000 t1=0x0000000000000000
> sbi_trap_error: hart0: t2=0x0000000000000000 t3=0x0000000000000000
> sbi_trap_error: hart0: t4=0x0000000000000000 t5=0x0000000000000000
> sbi_trap_error: hart0: t6=0x0000000000000000
>
> With this diff, I can successfully shut down the board. No idea if that
> is valid or not though.

The SiFive test is not a real device, so there is no documentation to
check against, at least none that I can find.

If the mainline Linux kernel does a 16-bit write then that should be
correct as it does the same thing on hardware and SiFive's
simulations.

Do you mind sending a patch?

Alistair

>
> Cheers,
> Nathan
>
> ============================================================
>
> diff --git a/hw/riscv/sifive_test.c b/hw/riscv/sifive_test.c
> index 0c78fb2c93..8c70dd69df 100644
> --- a/hw/riscv/sifive_test.c
> +++ b/hw/riscv/sifive_test.c
> @@ -59,7 +59,7 @@ static const MemoryRegionOps sifive_test_ops = {
>      .write = sifive_test_write,
>      .endianness = DEVICE_NATIVE_ENDIAN,
>      .valid = {
> -        .min_access_size = 4,
> +        .min_access_size = 2,
>          .max_access_size = 4
>      }
>  };
>
>

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Alistair Francis]

On Sun, Aug 30, 2020 at 12:24 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 30/08/2020 07:49, Nathan Chancellor wrote:
>
> > Unfortunately, it does not. I applied it on top of latest
> > git (ac8b279f13865d1a4f1958d3bf34240c1c3af90d) and I can still
> > reproduce my failure. Is it possible that type of fix is needed
> > in a RISC-V specific driver?
> >
> > Would you like me to comment on the Launchpad bug as well?
>
> Hi Nathan,
>
> I came up with a quick patch to enable some logging to catch memory accesses being
> refused for a similar bug report at
> https://bugs.launchpad.net/qemu/+bug/1886318/comments/13.

Can we apply this to QEMU master to print this is guest error logging
is on? This seems like a common-ish problem and it would be nice to
allow users to debug it themselves.

Alistair

>
> Can you apply the same change to your tree and report any messages on stderr as you
> do your board reboot test?
>
>
> ATB,
>
> Mark.
>

Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

[Michael S. Tsirkin]

On Wed, Aug 26, 2020 at 10:32:16PM -0700, Nathan Chancellor wrote:
> Hi all,
> 
> Sorry for the duplicate reply, my first one was rejected by a mailing
> list administrator for being too long so I resent it with the error logs
> as a link instead of inline.
> 
> On Wed, Jun 10, 2020 at 09:47:49AM -0400, Michael S. Tsirkin wrote:
> > Memory API documentation documents valid .min_access_size and .max_access_size
> > fields and explains that any access outside these boundaries is blocked.
> > 
> > This is what devices seem to assume.
> > 
> > However this is not what the implementation does: it simply
> > ignores the boundaries unless there's an "accepts" callback.
> > 
> > Naturally, this breaks a bunch of devices.
> > 
> > Revert to the documented behaviour.
> > 
> > Devices that want to allow any access can just drop the valid field,
> > or add the impl field to have accesses converted to appropriate
> > length.
> > 
> > Cc: qemu-stable@nongnu.org
> > Reviewed-by: Richard Henderson <rth@twiddle.net>
> > Fixes: CVE-2020-13754
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1842363
> > Fixes: a014ed07bd5a ("memory: accept mismatching sizes in memory_region_access_valid")
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  memory.c | 29 +++++++++--------------------
> >  1 file changed, 9 insertions(+), 20 deletions(-)
> > 
> > diff --git a/memory.c b/memory.c
> > index 91ceaf9fcf..3e9388fb74 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1352,35 +1352,24 @@ bool memory_region_access_valid(MemoryRegion *mr,
> >                                  bool is_write,
> >                                  MemTxAttrs attrs)
> >  {
> > -    int access_size_min, access_size_max;
> > -    int access_size, i;
> > +    if (mr->ops->valid.accepts
> > +        && !mr->ops->valid.accepts(mr->opaque, addr, size, is_write, attrs)) {
> > +        return false;
> > +    }
> >  
> >      if (!mr->ops->valid.unaligned && (addr & (size - 1))) {
> >          return false;
> >      }
> >  
> > -    if (!mr->ops->valid.accepts) {
> > +    /* Treat zero as compatibility all valid */
> > +    if (!mr->ops->valid.max_access_size) {
> >          return true;
> >      }
> >  
> > -    access_size_min = mr->ops->valid.min_access_size;
> > -    if (!mr->ops->valid.min_access_size) {
> > -        access_size_min = 1;
> > +    if (size > mr->ops->valid.max_access_size
> > +        || size < mr->ops->valid.min_access_size) {
> > +        return false;
> >      }
> > -
> > -    access_size_max = mr->ops->valid.max_access_size;
> > -    if (!mr->ops->valid.max_access_size) {
> > -        access_size_max = 4;
> > -    }
> > -
> > -    access_size = MAX(MIN(size, access_size_max), access_size_min);
> > -    for (i = 0; i < size; i += access_size) {
> > -        if (!mr->ops->valid.accepts(mr->opaque, addr + i, access_size,
> > -                                    is_write, attrs)) {
> > -            return false;
> > -        }
> > -    }
> > -
> >      return true;
> >  }
> >  
> > -- 
> > MST
> > 
> > 
> 
> I just ran into a regression with booting RISC-V kernels due to this
> commit. I can reproduce it with QEMU 5.1.0 and latest tip of tree
> (25f6dc28a3a8dd231c2c092a0e65bd796353c769 at the time of initially
> writing this).
> 
> The error message, commands, and bisect logs are available here:
> 
> https://gist.githubusercontent.com/nathanchance/c106dd22ec0c0e00f6a25daba106a1b9/raw/d929f2fff6da9126ded156affb0f19f359e9f693/qemu-5.1.0-issue-terminal-log.txt
> 
> I have attached the rootfs and kernel image used for these tests. If for
> some reason there is a problem receiving them, the kernel is just an
> arch/riscv/configs/defconfig kernel at Linux 5.9-rc2 and the rootfs is
> available here:
> 
> https://github.com/ClangBuiltLinux/boot-utils/blob/3b21a5b71451742866349ba4f18638c5a754e660/images/riscv/rootfs.cpio.zst
> 
> Please let me know if I can provide any follow up information or if I am
> doing something wrong.
> 
> Cheers,
> Nathan


So pls try this patch and use gdb backtrace to see access to which MR
triggers the assert.


diff --git a/softmmu/memory.c b/softmmu/memory.c
index 70b93104e8..dc8b7682aa 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1368,6 +1368,7 @@ bool memory_region_access_valid(MemoryRegion *mr,
 
     if (size > mr->ops->valid.max_access_size
         || size < mr->ops->valid.min_access_size) {
+        assert(0);
         return false;
     }
     return true;