From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:60439)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1aNpQP-00076Y-8S
	for qemu-devel@nongnu.org; Mon, 25 Jan 2016 17:15:54 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1aNpQL-0006CY-5w
	for qemu-devel@nongnu.org; Mon, 25 Jan 2016 17:15:53 -0500
Received: from mail-wm0-x243.google.com ([2a00:1450:400c:c09::243]:35235)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <paolo.bonzini@gmail.com>) id 1aNpQK-0006Bi-Un
	for qemu-devel@nongnu.org; Mon, 25 Jan 2016 17:15:49 -0500
Received: by mail-wm0-x243.google.com with SMTP id 123so13950462wmz.2
	for <qemu-devel@nongnu.org>; Mon, 25 Jan 2016 14:15:48 -0800 (PST)
Sender: Paolo Bonzini <paolo.bonzini@gmail.com>
References: <1453732190-13416-1-git-send-email-ppandit@redhat.com>
	<56A63347.2030103@redhat.com>
	<alpine.LFD.2.20.1601252317360.14976@wniryva>
From: Paolo Bonzini <pbonzini@redhat.com>
Message-ID: <56A69E91.4080607@redhat.com>
Date: Mon, 25 Jan 2016 23:15:45 +0100
MIME-Version: 1.0
In-Reply-To: <alpine.LFD.2.20.1601252317360.14976@wniryva>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit
Subject: Re: [Qemu-devel] [PATCH for v2.4.1] exec: fix a glitch in checking
	dma r/w access
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: P J P <ppandit@redhat.com>
Cc: Donghai Zdh <donghai.zdh@alibaba-inc.com>, QEMU Developers <qemu-devel@nongnu.org>, Peter Maydell <peter.maydell@linaro.org>



On 25/01/2016 19:19, P J P wrote:
> +-- On Mon, 25 Jan 2016, Paolo Bonzini wrote --+
> | >  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
> | >  {
> | >      if (memory_region_is_ram(mr)) {
> | > -        return !(is_write && mr->readonly);
> | > +        return (is_write && !mr->readonly);
> | 
> | Read or write?	Readonly?		Old		New
> |    Read	          Yes			 T		 F
> |    Read		  No			 T		 F
> |    Write		  Yes			 F		 F
> |    Write		  No			 T		 T
> | 
> | This patch changes behavior for reads (is_write=false).  For
> | address_space_read, this makes them go through a path that is at least
> | 100 times slower (memory_region_dispatch_read instead of just a memcpy).
> |  For address_space_map, it probably breaks everything that expects a
> | single block of RAM to be mapped in a single step, for example virtio.
> | 
> | So, how was this tested, and how can the bug be triggered?
> 
>   The bug was triggered if 'addr' in 'read_dword()' is set by user(ex. 
> 0xffffffff). The MemoryRegion section(*mr) could point to host memory area, 
> which is then copied by memcpy(2) call.

This should be handled correctly by address_space_translate_internal:

    if (memory_region_is_ram(mr)) {
        diff = int128_sub(section->size, int128_make64(addr));
        *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
    }

... then, on return from address_space_translate, l will be 1:

    e.g.  section->size = 0x100000000, addr = 0xffffffff;
          diff = 1;
          *plen = min(diff, *plen) = min(1, 4) = 1

> The patch was tested using gdb(1).

You also have to test that the patch doesn't break other code.  It's not
enough to test that it solves your problem.

Paolo
This bit hasn't changed since 2.4.1.