Re: [PATCH] memory: Revert "memory: accept mismatching sizes in memory_region_access_valid"

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Jun 10, 2020 at 09:47:52AM -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>

Actually, I rechecked and couldn't find this tag in my inbox.
So maybe this should have been:
Cc: Richard Henderson <rth@twiddle.net>
Or maybe I lost that email.

Richard could you ack this explicitly pls to avoid confusion?

> 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