Re: [PATCH] ntdll: round section parameters on 0x200
Andrey Turkin <[EMAIL PROTECTED]> writes: > I believe this is incorrect with regard to native Windows loader. As I > already said, I've tried to change PointerToRawData field on some files > with big FileAlignment and they load correctly if I or'ed them with some > value in range 0..0x1FF. Files refuses to load if I or'ed > PointerToRawData with 0x200 or 0x3FF! It sounds like what we really need here is some test cases. It shouldn't be hard to write a test that creates temp files with various header values, tries a LoadLibrary on them and checks the resulting memory contents. -- Alexandre Julliard [EMAIL PROTECTED]
Re: [PATCH] ntdll: round section parameters on 0x200
"Andrey Turkin" <[EMAIL PROTECTED]> wrote: -mapping->header_size = pos + size; +mapping->header_size = max( pos + size, nt.OptionalHeader.SizeOfHeaders ); You're reading my mind (or my hard drive), don't you? :) Ok, but if you going to change server code here, then you should probably align PointerToRawData in server code as well (writable shared sections will be mapped incorrectly otherwise). That was not a real fix but just a proof of (your) concept. I'd leave creation of a proper fix to Alexandre as he is the real expert in this area (that's why I sent an url of a smallest possible download with the packer itself (since it's self packed) to the list, so that an interested person could play with it). -- Dmitry.
Re: [PATCH] ntdll: round section parameters on 0x200
Some comments inside. Sorry for over-quoting :)
Dmitry Timoshkov wrote, on 11/13/06 10:19 MSK:
> "Andrey Turkin" <[EMAIL PROTECTED]> wrote:
>
>>> What is the file alignment of the problematic PE file? Is it 512
>>> (0x200) by any chance?
>>>
>> Yep. However, I've made some quick tests (that is, I've used PE tools
>> to rebuild some apps with larger file alignment and then tried to
>> change physical offset) and it seems that align cutoff is hard-coded
>> as 0x200.
>
> Well, of course larger alignments are always "aligned" to 0x200.
>
> I took your patch as a base and attached patch make upack 0.39 (latest
> available at its home page http://dwing.51.net/download/WinUpack39.rar)
> executable run under Wine.
>
> diff -up a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
> --- a/dlls/ntdll/virtual.c2006-11-08 13:55:07.0 +0800
> +++ b/dlls/ntdll/virtual.c2006-11-13 14:57:07.0 +0800
> @@ -133,8 +133,10 @@ static UINT_PTR page_mask;
> #define ROUND_ADDR(addr,mask) \
>((void *)((UINT_PTR)(addr) & ~(UINT_PTR)(mask)))
>
> -#define ROUND_SIZE(addr,size) \
> - (((UINT)(size) + ((UINT_PTR)(addr) & page_mask) + page_mask) &
> ~page_mask)
> +#define ROUND_UP(addr,size,mask) \
> + (((UINT)(size) + ((UINT_PTR)(addr) & (mask)) + (mask)) & ~(mask))
> +
> +#define ROUND_SIZE(addr,size) ROUND_UP(addr,size,page_mask)
>
> #define VIRTUAL_DEBUG_DUMP_VIEW(view) \
> do { if (TRACE_ON(virtual)) VIRTUAL_DumpView(view); } while (0)
> @@ -957,7 +959,7 @@ static NTSTATUS map_image( HANDLE hmappi
> if (!st.st_size) goto error;
> header_size = min( header_size, st.st_size );
> if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED
> | VPROT_READ,
> -removable ) != STATUS_SUCCESS) goto error;
> +TRUE ) != STATUS_SUCCESS) goto error;
> dos = (IMAGE_DOS_HEADER *)ptr;
> nt = (IMAGE_NT_HEADERS *)(ptr + dos->e_lfanew);
> header_end = ptr + ROUND_SIZE( 0, header_size );
> @@ -1031,7 +1033,7 @@ static NTSTATUS map_image( HANDLE hmappi
>
> for (i = pos = 0; i < nt->FileHeader.NumberOfSections; i++, sec++)
> {
> -SIZE_T map_size, file_size, end;
> +SIZE_T map_size, file_size, start, end;
>
> if (!sec->Misc.VirtualSize)
> {
> @@ -1045,6 +1047,7 @@ static NTSTATUS map_image( HANDLE hmappi
> }
>
> /* a few sanity checks */
> +start = (SIZE_T) ROUND_ADDR( sec->PointerToRawData,
> nt->OptionalHeader.FileAlignment - 1 );
I believe this is incorrect with regard to native Windows loader. As I
already said, I've tried to change PointerToRawData field on some files
with big FileAlignment and they load correctly if I or'ed them with some
value in range 0..0x1FF. Files refuses to load if I or'ed
PointerToRawData with 0x200 or 0x3FF!
> end = sec->VirtualAddress + ROUND_SIZE( sec->VirtualAddress,
> map_size );
> if (sec->VirtualAddress > total_size || end > total_size || end
> < sec->VirtualAddress)
> {
> @@ -1095,9 +1098,9 @@ static NTSTATUS map_image( HANDLE hmappi
> /* Note: if the section is not aligned properly
> map_file_into_view will magically
> * fall back to read(), so we don't need to check anything
> here.
> */
> -end = sec->PointerToRawData + file_size;
> -if (sec->PointerToRawData >= st.st_size || end > st.st_size ||
> end < sec->PointerToRawData ||
> -map_file_into_view( view, fd, sec->VirtualAddress,
> file_size, sec->PointerToRawData,
> +end = start + file_size;
> +if (start >= st.st_size || end > st.st_size || end < start ||
> +map_file_into_view( view, fd, sec->VirtualAddress,
> file_size, start,
> VPROT_COMMITTED | VPROT_READ |
> VPROT_WRITECOPY,
> removable ) != STATUS_SUCCESS)
> {
> @@ -1107,12 +1110,13 @@ static NTSTATUS map_image( HANDLE hmappi
>
> if (file_size & page_mask)
> {
> +start = ROUND_UP( 0, file_size,
> nt->OptionalHeader.FileAlignment - 1 );
I've not checked this, but there's probability that align is 0x200 here too.
> end = ROUND_SIZE( 0, file_size );
> if (end > map_size) end = map_size;
> TRACE_(module)("clearing %p - %p\n",
> - ptr + sec->VirtualAddress + file_size,
> + ptr + sec->VirtualAddress + start,
>ptr + sec->VirtualAddress + end );
> -memset( ptr + sec->VirtualAddress + file_size, 0, end -
> file_size );
> +memset( ptr + sec->VirtualAddress + start, 0, end - start );
> }
> }
>
> diff -up a/server/mapping.c b/server/mapping.c
> --- a/server/mapping.c2006-11-03 21:34:24.0 +0800
> +++ b/server/mapping.c2006-11-13 14:34:50.0 +0800
> @@ -243,7 +243,7 @@ static int get_image_params( struct mapp
>
> mapping->size= ROUND_SIZE( nt
Re: [PATCH] ntdll: round section parameters on 0x200
"Andrey Turkin" <[EMAIL PROTECTED]> wrote:
What is the file alignment of the problematic PE file? Is
it 512
(0x200) by any chance?
Yep. However, I've made some quick tests (that is, I've used PE tools to
rebuild some apps with larger file alignment and then tried to change
physical offset) and it seems that align cutoff is hard-coded as 0x200.
Well, of course larger alignments are always "aligned" to 0x200.
I took your patch as a base and attached patch make upack 0.39 (latest
available at its home page http://dwing.51.net/download/WinUpack39.rar)
executable run under Wine.
--
Dmitry.
diff -up a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
--- a/dlls/ntdll/virtual.c 2006-11-08 13:55:07.0 +0800
+++ b/dlls/ntdll/virtual.c 2006-11-13 14:57:07.0 +0800
@@ -133,8 +133,10 @@ static UINT_PTR page_mask;
#define ROUND_ADDR(addr,mask) \
((void *)((UINT_PTR)(addr) & ~(UINT_PTR)(mask)))
-#define ROUND_SIZE(addr,size) \
- (((UINT)(size) + ((UINT_PTR)(addr) & page_mask) + page_mask) & ~page_mask)
+#define ROUND_UP(addr,size,mask) \
+ (((UINT)(size) + ((UINT_PTR)(addr) & (mask)) + (mask)) & ~(mask))
+
+#define ROUND_SIZE(addr,size) ROUND_UP(addr,size,page_mask)
#define VIRTUAL_DEBUG_DUMP_VIEW(view) \
do { if (TRACE_ON(virtual)) VIRTUAL_DumpView(view); } while (0)
@@ -957,7 +959,7 @@ static NTSTATUS map_image( HANDLE hmappi
if (!st.st_size) goto error;
header_size = min( header_size, st.st_size );
if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED |
VPROT_READ,
-removable ) != STATUS_SUCCESS) goto error;
+TRUE ) != STATUS_SUCCESS) goto error;
dos = (IMAGE_DOS_HEADER *)ptr;
nt = (IMAGE_NT_HEADERS *)(ptr + dos->e_lfanew);
header_end = ptr + ROUND_SIZE( 0, header_size );
@@ -1031,7 +1033,7 @@ static NTSTATUS map_image( HANDLE hmappi
for (i = pos = 0; i < nt->FileHeader.NumberOfSections; i++, sec++)
{
-SIZE_T map_size, file_size, end;
+SIZE_T map_size, file_size, start, end;
if (!sec->Misc.VirtualSize)
{
@@ -1045,6 +1047,7 @@ static NTSTATUS map_image( HANDLE hmappi
}
/* a few sanity checks */
+start = (SIZE_T) ROUND_ADDR( sec->PointerToRawData,
nt->OptionalHeader.FileAlignment - 1 );
end = sec->VirtualAddress + ROUND_SIZE( sec->VirtualAddress, map_size );
if (sec->VirtualAddress > total_size || end > total_size || end <
sec->VirtualAddress)
{
@@ -1095,9 +1098,9 @@ static NTSTATUS map_image( HANDLE hmappi
/* Note: if the section is not aligned properly map_file_into_view will
magically
* fall back to read(), so we don't need to check anything here.
*/
-end = sec->PointerToRawData + file_size;
-if (sec->PointerToRawData >= st.st_size || end > st.st_size || end <
sec->PointerToRawData ||
-map_file_into_view( view, fd, sec->VirtualAddress, file_size,
sec->PointerToRawData,
+end = start + file_size;
+if (start >= st.st_size || end > st.st_size || end < start ||
+map_file_into_view( view, fd, sec->VirtualAddress, file_size,
start,
VPROT_COMMITTED | VPROT_READ | VPROT_WRITECOPY,
removable ) != STATUS_SUCCESS)
{
@@ -1107,12 +1110,13 @@ static NTSTATUS map_image( HANDLE hmappi
if (file_size & page_mask)
{
+start = ROUND_UP( 0, file_size, nt->OptionalHeader.FileAlignment -
1 );
end = ROUND_SIZE( 0, file_size );
if (end > map_size) end = map_size;
TRACE_(module)("clearing %p - %p\n",
- ptr + sec->VirtualAddress + file_size,
+ ptr + sec->VirtualAddress + start,
ptr + sec->VirtualAddress + end );
-memset( ptr + sec->VirtualAddress + file_size, 0, end - file_size
);
+memset( ptr + sec->VirtualAddress + start, 0, end - start );
}
}
diff -up a/server/mapping.c b/server/mapping.c
--- a/server/mapping.c 2006-11-03 21:34:24.0 +0800
+++ b/server/mapping.c 2006-11-13 14:34:50.0 +0800
@@ -243,7 +243,7 @@ static int get_image_params( struct mapp
mapping->size= ROUND_SIZE( nt.OptionalHeader.SizeOfImage );
mapping->base= (void *)nt.OptionalHeader.ImageBase;
-mapping->header_size = pos + size;
+mapping->header_size = max( pos + size, nt.OptionalHeader.SizeOfHeaders );
mapping->protect = VPROT_IMAGE;
/* sanity check */
Re: [PATCH] ntdll: round section parameters on 0x200
Dmitry Timoshkov wrote: "Andrey Turkin" <[EMAIL PROTECTED]> wrote: if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED | VPROT_READ, -removable ) != STATUS_SUCCESS) goto error; +TRUE ) != STATUS_SUCCESS) goto error; This chunk has nothin to do with the patch description and simply is wrong. I've hardcoded removevable as TRUE here to force map_file_into_view to read data and not mmap it (because mmap will map whole 4k page). Why is it wrong? Some packers depend on this. As I said in patch description, an alternative would be memset of area beyond header (which would lead to mmap, then COW a page and then memset of almost 4k). I reread your explanations and I see now that somehow I misinterpreted your reasoning. What is the file alignment of the problematic PE file? Is it 512 (0x200) by any chance? Yep. However, I've made some quick tests (that is, I've used PE tools to rebuild some apps with larger file alignment and then tried to change physical offset) and it seems that align cutoff is hard-coded as 0x200.
Re: [PATCH] ntdll: round section parameters on 0x200
"Andrey Turkin" <[EMAIL PROTECTED]> wrote: if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED | VPROT_READ, -removable ) != STATUS_SUCCESS) goto error; +TRUE ) != STATUS_SUCCESS) goto error; This chunk has nothin to do with the patch description and simply is wrong. I've hardcoded removevable as TRUE here to force map_file_into_view to read data and not mmap it (because mmap will map whole 4k page). Why is it wrong? Some packers depend on this. As I said in patch description, an alternative would be memset of area beyond header (which would lead to mmap, then COW a page and then memset of almost 4k). I reread your explanations and I see now that somehow I misinterpreted your reasoning. What is the file alignment of the problematic PE file? Is it 512 (0x200) by any chance? -- Dmitry.
Re: [PATCH] ntdll: round section parameters on 0x200
Dmitry Timoshkov wrote: "Andrey Turkin" <[EMAIL PROTECTED]> wrote: if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED | VPROT_READ, -removable ) != STATUS_SUCCESS) goto error; +TRUE ) != STATUS_SUCCESS) goto error; This chunk has nothin to do with the patch description and simply is wrong. I've hardcoded removevable as TRUE here to force map_file_into_view to read data and not mmap it (because mmap will map whole 4k page). Why is it wrong? Some packers depend on this. As I said in patch description, an alternative would be memset of area beyond header (which would lead to mmap, then COW a page and then memset of almost 4k). -sec->PointerToRawData, (int)pos, file_size, map_size, +(int)start, (int)pos, file_size, map_size, ... -sec->PointerToRawData, sec->SizeOfRawData, +(int)start, sec->SizeOfRawData, Please use a proper format specifier instead of a cast. Can do (actually I realized that SIZE_T is not so good for "start"; DWORD would be better).
Re: [PATCH] ntdll: round section parameters on 0x200
"Andrey Turkin" <[EMAIL PROTECTED]> wrote: if (map_file_into_view( view, fd, 0, header_size, 0, VPROT_COMMITTED | VPROT_READ, -removable ) != STATUS_SUCCESS) goto error; +TRUE ) != STATUS_SUCCESS) goto error; This chunk has nothin to do with the patch description and simply is wrong. -sec->PointerToRawData, (int)pos, file_size, map_size, +(int)start, (int)pos, file_size, map_size, ... -sec->PointerToRawData, sec->SizeOfRawData, +(int)start, sec->SizeOfRawData, Please use a proper format specifier instead of a cast. -- Dmitry.
