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
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-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; > } > > Queued, thanks. Paolo
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 o � RISCV RSC ���� ������ �.�sVV� ����M�uE�1�M�s s �sU���A�ss ��s@s@������bs�� ��fs�V�5 � ���� ��G�6���1 � ������ � 3 g 9 �sUs ��s P�� s@s@������bs��BcDU o���F��� ����F�ǚc�� #� �����*���&�1 ��&�� �v� ����?���o�� ��#$1 ��� `+� @E�������fs�V�5 �� ����� F���6�� 2 ��� � �o�O�A"� "d�G##��EA��A"� "d�GG##��EA��A"� "d�GG#'��EA��A"�� ����E� ~#<��@K ���`dEA��A"�� ����E� �z�#;�"�@K 瀠�`dEA��A"� "dEA��]q��&�J�N�R�V�Z�^��*����$ ��Č�+ ���;۳ �2�ڳ ���0ڳ �0c�t�H��օJ���J ������`J�� "