On Thu, Aug 27, 2020 at 08:53:30AM -0700, Alistair Francis wrote: > On Wed, Aug 26, 2020 at 11:26 PM Nathan Chancellor > <natechancel...@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-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 > > 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
