On 10/06/20 15:47, 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; > } > >
Queued, thanks. Paolo
