[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_r
From: |
Nathan Chancellor |
Subject: |
Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid" |
Date: |
Sat, 29 Aug 2020 23:49:44 -0700 |
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", Michael S. Tsirkin, 2020/08/30