diff mbox series

memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

Message ID 20200610134731.1514409-1-mst@redhat.com
State New
Headers show
Series memory: Revert "memory: accept mismatching sizes in memory_region_access_valid" | expand

Commit Message

Michael S. Tsirkin June 10, 2020, 1:47 p.m. UTC 
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(-)

Comments

Michael S. Tsirkin Aug. 30, 2020, 6:20 a.m. UTC | #1 
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
Nathan Chancellor Aug. 30, 2020, 6:49 a.m. UTC | #2 
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
> 
> 
>
Nathan Chancellor Aug. 30, 2020, 7:43 a.m. UTC | #3 
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
     }
 };
Alistair Francis Aug. 31, 2020, 4:08 p.m. UTC | #4 
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.
>
diff mbox series

Patch

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;
 }