Re: [Qemu-devel] [PATCH] mmap: add check if requested memory area fits target address space

[andrzej zaborowski]

2008/11/10 Kirill A. Shutemov <address@hidden>:
> 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 <address@hidden>:
>> > On Mon, Oct 27, 2008 at 08:37:39PM +0100, andrzej zaborowski wrote:
>> >> 2008/10/27 Kirill A. Shutemov <address@hidden>:
>> >> > On Mon, Oct 27, 2008 at 02:08:52PM +0100, andrzej zaborowski wrote:
>> >> >> On 17/10/2008, Kirill A. Shutemov <address@hidden> wrote:
>> >> >> > Signed-off-by: Kirill A. Shutemov <address@hidden>
>> >> >> >  ---
>> >> >> >   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