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