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-sta...@nongnu.org > > > Reviewed-by: Richard Henderson <r...@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 <m...@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 > > >
