[PATCH] ntdll: Improve handling of THUMB_MOV32 relocations

[André Hentschel]

Am 02.01.2014 21:57, schrieb Martin Storsjö:
> On Thu, 2 Jan 2014, André Hentschel wrote:
> 
>> Am 02.01.2014 17:01, schrieb Martin Storsjo:
>>> Since the delta variable actually is signed, the delta > 0xffff
>>> condition could be false even if the high part of the delta
>>> was nonzero, ending up with not updating the high half of the
>>> relocation at all.
>>>
>>> Additionally, carry any overflow from the low word into the
>>> high word delta.
>>
>>
>> Hi,
>> Do you have an App that triggers that?
> 
> I've got a 5 MB .exe that triggers this occasionally. If I disable ASLR in the kernel, it doesn't trigger. I guess the bigger the image, the larger the risk that ASLR will force the exe to be relocated, triggering the issue.
> 
> Conversely, if I change map_image to not try to load the image at the base address (as if it would have failed to load at that address due to ASLR), this triggers every time even with a small hello world .exe.
> 
>> So you mean the case when the sign bit is used, e.g. 10101010b... Then the delta is negative, but is everything set correctly in the instruction?
> 
> Yes, the calculation seem to work just fine with my patch applied. With the 5 MB .exe, the relocation that is done on startup was:
> 
> trace:module:map_image relocating from 0x400000-0xe6e000 to 0xb58a0000-0xb630e000
> 
> So the delta really isn't negative per se, but it's too large to represent with a 32 bit signed integer.
> 
> 
> So to reproduce easily, apply this diff to your tree, then try to execute http://albin.abo.fi/~mstorsjo/helloworld-arm.exe (a plain hello world exe compiled with msvc 2012 for winrt). At least here, it tries to relocate the .exe from 0x400000-0x410000 to 0xb6200000-0xb6210000.
> 
> index e93ce2e..a6ffe38 100644
> --- a/dlls/ntdll/virtual.c
> +++ b/dlls/ntdll/virtual.c
> @@ -1077,9 +1077,6 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
> 
>      server_enter_uninterrupted_section( &csVirtual, &sigset );
> 
> -    if (base >= (char *)address_space_start)  /* make sure the DOS area remains free */
> -        status = map_view( &view, base, total_size, mask, FALSE,
> -                           VPROT_COMMITTED | VPROT_READ | VPROT_EXEC | VPROT_WRITECOPY | VPROT_IMAGE );
> 
>      if (status != STATUS_SUCCESS)
>          status = map_view( &view, NULL, total_size, mask, FALSE,
> 
> 
> 
>>> ---
>>>  dlls/ntdll/loader.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/dlls/ntdll/loader.c b/dlls/ntdll/loader.c
>>> index e0907e7..1f13424 100644
>>> --- a/dlls/ntdll/loader.c
>>> +++ b/dlls/ntdll/loader.c
>>> @@ -2230,19 +2230,19 @@ IMAGE_BASE_RELOCATION * WINAPI LdrProcessRelocationBlock( void *page, UINT count
>>>              DWORD inst = *(INT_PTR *)((char *)page + offset);
>>>              DWORD imm16 = ((inst << 1) & 0x0800) + ((inst << 12) & 0xf000) +
>>>                            ((inst >> 20) & 0x0700) + ((inst >> 16) & 0x00ff);
>>> +            int hi_delta;
>>
>> Why you're using int here? DWORD looks much better for this.
> 
> No particular reason, DWORD should work just fine as well, it seems. I can send a new patch with that changed if you're ok with the rest of it - just let me know.

You've done your homework :) so yeah, DWORD would be better imho.