From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1KzW8d-0005zy-OM for qemu-devel@nongnu.org; Mon, 10 Nov 2008 07:45:03 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1KzW8c-0005zi-Ar for qemu-devel@nongnu.org; Mon, 10 Nov 2008 07:45:03 -0500 Received: from [199.232.76.173] (port=40652 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1KzW8c-0005zf-35 for qemu-devel@nongnu.org; Mon, 10 Nov 2008 07:45:02 -0500 Received: from rv-out-0708.google.com ([209.85.198.245]:25520) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1KzW8b-0007XD-K9 for qemu-devel@nongnu.org; Mon, 10 Nov 2008 07:45:01 -0500 Received: by rv-out-0708.google.com with SMTP id f25so2360922rvb.22 for ; Mon, 10 Nov 2008 04:45:00 -0800 (PST) Message-ID: Date: Mon, 10 Nov 2008 13:45:00 +0100 From: "andrzej zaborowski" Subject: Re: [Qemu-devel] [PATCH] mmap: add check if requested memory area fits target address space In-Reply-To: <20081110055540.GA2423@localhost.localdomain> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <1223892640-15545-13-git-send-email-kirill@shutemov.name> <1224225264-8483-1-git-send-email-kirill@shutemov.name> <20081027154835.GA10763@localhost.localdomain> <20081027200654.GC10763@localhost.localdomain> <20081110055540.GA2423@localhost.localdomain> Reply-To: qemu-devel@nongnu.org List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Kirill A. Shutemov" Cc: qemu-devel@nongnu.org 2008/11/10 Kirill A. Shutemov : > On Mon, Nov 10, 2008 at 04:30:39AM +0100, andrzej zaborowski wrote: >> Sorry to resurrect this old thread, I still can't convince myself. >> >> 2008/10/27 Kirill A. Shutemov : >> > On Mon, Oct 27, 2008 at 08:37:39PM +0100, andrzej zaborowski wrote: >> >> 2008/10/27 Kirill A. Shutemov : >> >> > On Mon, Oct 27, 2008 at 02:08:52PM +0100, andrzej zaborowski wrote: >> >> >> On 17/10/2008, Kirill A. Shutemov wrote: >> >> >> > Signed-off-by: Kirill A. Shutemov >> >> >> > --- >> >> >> > linux-user/mmap.c | 5 +++++ >> >> >> > 1 files changed, 5 insertions(+), 0 deletions(-) >> >> >> > >> >> >> > diff --git a/linux-user/mmap.c b/linux-user/mmap.c >> >> >> > index bc20f4b..9a2f355 100644 >> >> >> > --- a/linux-user/mmap.c >> >> >> > +++ b/linux-user/mmap.c >> >> >> > @@ -388,6 +388,11 @@ abi_long target_mmap(abi_ulong start, abi_ulong len, int prot, >> >> >> > end = start + len; >> >> >> > real_end = HOST_PAGE_ALIGN(end); >> >> >> > >> >> >> > + if ((unsigned long)start + len > (abi_ulong) -1) { >> >> >> > + errno = EINVAL; >> >> >> > + goto fail; >> >> >> > + } >> >> >> >> >> >> I'm being picky but this would prevent the last byte from being used? >> >> >> :p (or the last page because len is aligned?) >> >> > >> >> > No, it returns error if start + len is more than 0xFFFFFFFF (32-bit >> >> > target). >> >> > >> >> >> >> >> >> I'm not sure unsigned long is the best choice. >> >> > >> >> > Why? >> >> >> >> I may be misunderstanding but I think the range of valid addresses >> >> should depend on target word size, not host (even if the combination >> >> where it matters is not yet supported). >> > >> > start + len can be more than 0xFFFFFFFF ((abi_ulong) -1) on 32-bit targets, >> > so we should use host's long. >> > >> >> On a 32-bit host the condition is always false. >> > >> > It's ok. It can be true, only on 64-bit host. >> >> Let's say we have a 32-bit host and target, the call receives start == >> 0xffff0050 and len == 0x100000, the check passes, when it shouldn't >> (?). On a 64-bit host it would fail, but this check should be >> independent of the host type. >> (It'll probably fail later in the host mmap() -- but in the meantime >> mmap_frag() might succeed for example) > > mmap_frag() will not be called if host mmap() fail. mmap can fail on many > conditions, it's one of them. Note that mmap() is called at the end, after mmap'ping the start and the end chunks. > > Probably, I should add comment to this check, that it's for 64-bit host > only. Ok? This doesn't make it independent of the host type :p Cheers