CODEWITHSUNDEEP
cwindowskernel
user918176
user918176

Reputation: 1800

RtlCopyMemory PUNICODE_STRING to WCHAR segfaults

I am essentially converting PUNICODE_STRING to constant size WCHAR[] in my kernel driver, trying to avoid overflow. My approach works 99% of the time, but sometimes I get segmentation fault from the first RtlCopyMemory, most useful output from the debugger is something like:

PAGE_FAULT_IN_NONPAGED_AREA (50)
Invalid system memory was referenced.  This cannot be protected by try-except,
it must be protected by a Probe.  Typically the address is just plain bad or it
is pointing at freed memory.
Arguments:
Arg1: ffffc0016c0dd000, memory referenced.
Arg2: 0000000000000000, value 0 = read operation, 1 = write operation.
Arg3: fffff800adea85bd, If non-zero, the instruction address which referenced the bad memory
    address.
Arg4: 0000000000000000, (reserved)

What is wrong with my approach (see below for example)? 

typedef struct _MYRESULT {
 WCHAR Result[512];
} MYRESULT;

void myFunction(
_Inout_ MYRESULT *result,
_In_ PUNICODE_STRING s
)
{
 if (s->Length < 511) {
  // It is usually this RtlCopyMemory that crashes the kernel
  RtlCopyMemory(result->Result, s->Buffer, sizeof(WCHAR)*s->Length);
  // NULL terminate
  result->Result[(sizeof)WCHAR)*s->Length)] = 0;
 } else {
  RtlCopyMemory(result->Result, s->Buffer, sizeof(WCHAR)*512);
  // NULL terminate
  result->Result[511] = 0;
 }
}

Upvotes: 0

Views: 1520

Answers (1)

alk
alk

Reputation: 70971

The code copies too much data.

From MSDN on UNICODE_STRING (emphasis by me):

Length

Specifies the length, in bytes, of the string pointed to by the Buffer member, not including the terminating NULL character, if any.

From the calls to RtlCopyMemory() remove the sizeof(WCHAR).

void myFunction(
_Inout_ MYRESULT *result,
_In_ PUNICODE_STRING s
)

{
  if (s->Length < (511 * sizeof * result)) {
     // It is usually this RtlCopyMemory that crashes the kernel
     RtlCopyMemory(result->Result, s->Buffer, s->Length);
     // NULL terminate
     result->Result[(sizeof)WCHAR)*s->Length)] = 0;
   } else {
     RtlCopyMemory(result->Result, s->Buffer, 512);
      // NULL terminate
     result->Result[511 * sizeof * result] = 0;
   }
}

Or less error prone (also following the DRY principle):

#define RESULT_MAXIMUMLENGTH (511) /* This needs to stay smaller 
                                      then USHORT_MAX / sizeof * MYRESULT.Result. */

typedef struct _MYRESULT {
  WCHAR Result[RESULT_MAXIMUMLENGTH + 1];
} MYRESULT;

void myFunction(
_Inout_ MYRESULT *result,
_In_ PUNICODE_STRING s
)
{
  USHORT len = (s->Length < (RESULT_MAXIMUMLENGTH * sizeof * result)) 
    ? s->Length 
    : RESULT_MAXIMUMLENGTH * sizeof * result;

  RtlCopyMemory(result->Result, s->Buffer, len);
  result->Result[len] = 0;
}

Lesson learned: RTFM helps.

Upvotes: 3

Related Questions