Re: [PATCH] ntdll: deal with images which override the section table during mapping

[Marcus Meissner]

On Mon, Oct 10, 2011 at 11:36:33AM +0200, Bernhard Loos wrote:
> dlls/ntdll/virtual.c |   38 ++++++++++++++++++++++++++++++++++----
>  1 files changed, 34 insertions(+), 4 deletions(-)

I think this needs tests, the algorithms are very fragile.

Ciao, Marcus
> diff --git a/dlls/ntdll/virtual.c b/dlls/ntdll/virtual.c
> index 5e69eb9..7841d9c 100644
> --- a/dlls/ntdll/virtual.c
> +++ b/dlls/ntdll/virtual.c
> @@ -1114,6 +1114,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      IMAGE_DOS_HEADER *dos;
>      IMAGE_NT_HEADERS *nt;
>      IMAGE_SECTION_HEADER *sec;
> - IMAGE_SECTION_HEADER *section_table = NULL;
> IMAGE_DATA_DIRECTORY *imports;
> NTSTATUS status = STATUS_CONFLICTING_ADDRESSES;
> int i;
>
> @@ -1123,6 +1124,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      struct file_view *view = NULL;
>      char *ptr, *header_end;
>      INT_PTR delta = 0;
> -
>
> DWORD SizeOfHeaders;
>
> /* zero-map the whole range */
>
> @@ -1158,8 +1160,11 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      header_end = ptr + ROUND_SIZE( 0, header_size );
>      memset( ptr + header_size, 0, header_end - (ptr + header_size) );
>      if ((char *)(nt + 1) > header_end) goto error;
> - sec = (IMAGE_SECTION_HEADER*)((char*)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader);
> - if ((char *)(sec + nt->FileHeader.NumberOfSections) > header_end) goto error;
> -
>
> section_table = (IMAGE_SECTION_HEADER*)((char*)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader);
> -
>
> /* make sure we mapped enough memory */
> -
>
> if ((char *)(section_table + nt->FileHeader.NumberOfSections) > header_end) goto error;
> -
>
> /* windows is only interested in the size as given by OptionalHeader.SizeOfHeaders */
> -
>
> SizeOfHeaders = ROUND_SIZE( 0, nt->OptionalHeader.SizeOfHeaders );
>
> imports = nt->OptionalHeader.DataDirectory + IMAGE_DIRECTORY_ENTRY_IMPORT;
> if (!imports->Size || !imports->VirtualAddress) imports = NULL;
>
> @@ -1180,7 +1185,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>          if (nt->OptionalHeader.FileAlignment != nt->OptionalHeader.SectionAlignment) goto error;
>          for (i = 0; i < nt->FileHeader.NumberOfSections; i++)
>          {
> -
>        if (sec[i].VirtualAddress != sec[i].PointerToRawData)
> -
>        if (section_table[i].VirtualAddress != section_table[i].PointerToRawData)
>            goto error;  /* Windows refuses to load in that case too */
>    }
>
> @@ -1192,6 +1197,24 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>          goto done;
>      }
> -
>
> if ((char *)(section_table + nt->FileHeader.NumberOfSections) > ptr + SizeOfHeaders)
> -
>
> {
> -
>    /* part of the section table is outside of SizeOfHeaders
> -
>     * at this point, it's likely that a section will get mapped over
> -
>     * the section table (Borderlands does this)
> -
>     * Not sure how it works on Windows, but we make a copy of the
> -
>     * table at this point */
> -
>     sec = RtlAllocateHeap( virtual_heap, 0, sizeof(*sec) * nt->FileHeader.NumberOfSections);
> -
>     if (!sec)
> -
>     {
> -
>         status = STATUS_NO_MEMORY;
> -
>         goto error;
> -
>     }
> -
>     memcpy( sec, section_table, sizeof(*sec) * nt->FileHeader.NumberOfSections);
> -
>     section_table = sec;
> -
> }
> -
> else
> -
>    sec = section_table;
>
> /* map all the sections */
>
> @@ -1200,6 +1223,9 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>          static const SIZE_T sector_align = 0x1ff;
>          SIZE_T map_size, file_start, file_size, end;
> -
>    if (sec->VirtualAddress < SizeOfHeaders)
> -
>        goto error;
> -
>    if (!sec->Misc.VirtualSize)
>        map_size = ROUND_SIZE( 0, sec->SizeOfRawData );
>    else
>
> @@ -1329,7 +1355,7 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>  VIRTUAL_SetProt( view, ptr, ROUND_SIZE( 0, header_size ), VPROT_COMMITTED | VPROT_READ );
>
> - sec = (IMAGE_SECTION_HEADER*)((char *)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader);
> - sec = section_table;
> for (i = 0; i < nt->FileHeader.NumberOfSections; i++, sec++)
> {
>     SIZE_T size;
>
> @@ -1361,6 +1387,8 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>      }
>
> done:
> - if (section_table && (char *)section_table != (char *)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader)
> -
>    RtlFreeHeap( virtual_heap, 0, section_table );
> view->mapping = dup_mapping;
>    server_leave_uninterrupted_section( &csVirtual, &sigset );
>
> @@ -1373,6 +1401,8 @@ static NTSTATUS map_image( HANDLE hmapping, int fd, char *base, SIZE_T total_siz
>
> error:
>      if (view) delete_view( view );
> - if (section_table && (char *)section_table != (char *)&nt->OptionalHeader+nt->FileHeader.SizeOfOptionalHeader)
> -
>    RtlFreeHeap( virtual_heap, 0, section_table );
> server_leave_uninterrupted_section( &csVirtual, &sigset );
>    if (dup_mapping) NtClose( dup_mapping );
>    return status;
>