[PATCH v2 0/1] MR7357: rpcrt4: Avoid unsigned overflows in buffer validation.
Fixes Clang 20 `-Wtautological-compare` warnings. Based on patch by William Horvath. -- v2: rpcrt4: Don't validate buffer in NDR marshaler. https://gitlab.winehq.org/wine/wine/-/merge_requests/7357
From: Jacek Caban <jacek(a)codeweavers.com> --- dlls/rpcrt4/ndr_marshall.c | 62 ++++++++++++++------------------ dlls/rpcrt4/tests/ndr_marshall.c | 33 +++++++++++++++++ 2 files changed, 60 insertions(+), 35 deletions(-) diff --git a/dlls/rpcrt4/ndr_marshall.c b/dlls/rpcrt4/ndr_marshall.c index c68eb0ee57e..60b44f27d48 100644 --- a/dlls/rpcrt4/ndr_marshall.c +++ b/dlls/rpcrt4/ndr_marshall.c @@ -746,16 +746,8 @@ static inline void safe_copy_from_buffer(MIDL_STUB_MESSAGE *pStubMsg, void *p, U } /* copies data to the buffer, checking that there is enough space to do so */ -static inline void safe_copy_to_buffer(MIDL_STUB_MESSAGE *pStubMsg, const void *p, ULONG size) +static inline void copy_to_buffer(MIDL_STUB_MESSAGE *pStubMsg, const void *p, ULONG size) { - if ((pStubMsg->Buffer + size < pStubMsg->Buffer) || /* integer overflow of pStubMsg->Buffer */ - (pStubMsg->Buffer + size > (unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength)) - { - ERR("buffer overflow - Buffer = %p, BufferEnd = %p, size = %lu\n", - pStubMsg->Buffer, (unsigned char *)pStubMsg->RpcMsg->Buffer + pStubMsg->BufferLength, - size); - RpcRaiseException(RPC_X_BAD_STUB_DATA); - } memcpy(pStubMsg->Buffer, p, size); pStubMsg->Buffer += size; } @@ -1547,7 +1539,7 @@ unsigned char * WINAPI NdrPointerMarshall(PMIDL_STUB_MESSAGE pStubMsg, { align_pointer_clear(&pStubMsg->Buffer, 4); Buffer = pStubMsg->Buffer; - safe_buffer_increment(pStubMsg, 4); + pStubMsg->Buffer += 4; } else Buffer = pStubMsg->Buffer; @@ -1749,7 +1741,7 @@ unsigned char * WINAPI NdrSimpleStructMarshall(PMIDL_STUB_MESSAGE pStubMsg, align_pointer_clear(&pStubMsg->Buffer, pFormat[1] + 1); pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory, size); + copy_to_buffer(pStubMsg, pMemory, size); if (pFormat[0] != FC_STRUCT) EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat+4); @@ -2036,7 +2028,7 @@ static inline void array_write_variance_and_marshall( size = safe_multiply(esize, pStubMsg->MaxCount); if (fHasPointers) pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory, size); + copy_to_buffer(pStubMsg, pMemory, size); if (fHasPointers) EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat); @@ -2056,7 +2048,7 @@ static inline void array_write_variance_and_marshall( if (fHasPointers) pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, size); + copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, size); if (fHasPointers) EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat); @@ -2071,7 +2063,7 @@ static inline void array_write_variance_and_marshall( WriteVariance(pStubMsg); size = safe_multiply(esize, pStubMsg->ActualCount); - safe_copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */ + copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */ break; case FC_BOGUS_ARRAY: alignment = pFormat[1] + 1; @@ -2612,7 +2604,7 @@ unsigned char * WINAPI NdrNonConformantStringMarshall(PMIDL_STUB_MESSAGE pStubM WriteVariance(pStubMsg); size = safe_multiply(esize, pStubMsg->ActualCount); - safe_copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */ + copy_to_buffer(pStubMsg, pMemory, size); /* the string itself */ return NULL; } @@ -2855,14 +2847,14 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg, case FC_SMALL: case FC_USMALL: TRACE("byte=%d <= %p\n", *(WORD*)pMemory, pMemory); - safe_copy_to_buffer(pStubMsg, pMemory, 1); + copy_to_buffer(pStubMsg, pMemory, 1); pMemory += 1; break; case FC_WCHAR: case FC_SHORT: case FC_USHORT: TRACE("short=%d <= %p\n", *(WORD*)pMemory, pMemory); - safe_copy_to_buffer(pStubMsg, pMemory, 2); + copy_to_buffer(pStubMsg, pMemory, 2); pMemory += 2; break; case FC_ENUM16: @@ -2871,7 +2863,7 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg, TRACE("enum16=%ld <= %p\n", *(DWORD*)pMemory, pMemory); if (32767 < *(DWORD*)pMemory) RpcRaiseException(RPC_X_ENUM_VALUE_OUT_OF_RANGE); - safe_copy_to_buffer(pStubMsg, &val, 2); + copy_to_buffer(pStubMsg, &val, 2); pMemory += 4; break; } @@ -2879,7 +2871,7 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg, case FC_ULONG: case FC_ENUM32: TRACE("long=%ld <= %p\n", *(DWORD*)pMemory, pMemory); - safe_copy_to_buffer(pStubMsg, pMemory, 4); + copy_to_buffer(pStubMsg, pMemory, 4); pMemory += 4; break; case FC_INT3264: @@ -2887,23 +2879,23 @@ static unsigned char * ComplexMarshall(PMIDL_STUB_MESSAGE pStubMsg, { UINT val = *(UINT_PTR *)pMemory; TRACE("int3264=%Id <= %p\n", *(UINT_PTR *)pMemory, pMemory); - safe_copy_to_buffer(pStubMsg, &val, sizeof(UINT)); + copy_to_buffer(pStubMsg, &val, sizeof(UINT)); pMemory += sizeof(UINT_PTR); break; } case FC_FLOAT: TRACE("float=%f <= %p\n", *(float*)pMemory, pMemory); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(float)); + copy_to_buffer(pStubMsg, pMemory, sizeof(float)); pMemory += sizeof(float); break; case FC_HYPER: TRACE("longlong=%s <= %p\n", wine_dbgstr_longlong(*(ULONGLONG*)pMemory), pMemory); - safe_copy_to_buffer(pStubMsg, pMemory, 8); + copy_to_buffer(pStubMsg, pMemory, 8); pMemory += 8; break; case FC_DOUBLE: TRACE("double=%f <= %p\n", *(double*)pMemory, pMemory); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(double)); + copy_to_buffer(pStubMsg, pMemory, sizeof(double)); pMemory += sizeof(double); break; case FC_RP: @@ -4744,7 +4736,7 @@ unsigned char * WINAPI NdrConformantStructMarshall(PMIDL_STUB_MESSAGE pStubMsg, } /* copy constant sized part of struct */ pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory, pCStructFormat->memory_size + bufsize); + copy_to_buffer(pStubMsg, pMemory, pCStructFormat->memory_size + bufsize); if (pCStructFormat->type == FC_CPSTRUCT) EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat); @@ -4950,7 +4942,7 @@ unsigned char * WINAPI NdrConformantVaryingStructMarshall(PMIDL_STUB_MESSAGE pS /* write constant sized part */ pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory, pCVStructFormat->memory_size); + copy_to_buffer(pStubMsg, pMemory, pCVStructFormat->memory_size); array_write_variance_and_marshall(*pCVArrayFormat, pStubMsg, pMemory + pCVStructFormat->memory_size, @@ -5199,7 +5191,7 @@ unsigned char * WINAPI NdrFixedArrayMarshall(PMIDL_STUB_MESSAGE pStubMsg, } pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory, total_size); + copy_to_buffer(pStubMsg, pMemory, total_size); pFormat = EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat); @@ -5426,7 +5418,7 @@ unsigned char * WINAPI NdrVaryingArrayMarshall(PMIDL_STUB_MESSAGE pStubMsg, bufsize = safe_multiply(esize, pStubMsg->ActualCount); pStubMsg->BufferMark = pStubMsg->Buffer; - safe_copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, bufsize); + copy_to_buffer(pStubMsg, pMemory + pStubMsg->Offset, bufsize); EmbeddedPointerMarshall(pStubMsg, pMemory, pFormat); @@ -6626,14 +6618,14 @@ static unsigned char *WINAPI NdrBaseTypeMarshall( case FC_CHAR: case FC_SMALL: case FC_USMALL: - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(UCHAR)); + copy_to_buffer(pStubMsg, pMemory, sizeof(UCHAR)); TRACE("value: 0x%02x\n", *pMemory); break; case FC_WCHAR: case FC_SHORT: case FC_USHORT: align_pointer_clear(&pStubMsg->Buffer, sizeof(USHORT)); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(USHORT)); + copy_to_buffer(pStubMsg, pMemory, sizeof(USHORT)); TRACE("value: 0x%04x\n", *(USHORT *)pMemory); break; case FC_LONG: @@ -6641,20 +6633,20 @@ static unsigned char *WINAPI NdrBaseTypeMarshall( case FC_ERROR_STATUS_T: case FC_ENUM32: align_pointer_clear(&pStubMsg->Buffer, sizeof(ULONG)); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(ULONG)); + copy_to_buffer(pStubMsg, pMemory, sizeof(ULONG)); TRACE("value: 0x%08lx\n", *(ULONG *)pMemory); break; case FC_FLOAT: align_pointer_clear(&pStubMsg->Buffer, sizeof(float)); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(float)); + copy_to_buffer(pStubMsg, pMemory, sizeof(float)); break; case FC_DOUBLE: align_pointer_clear(&pStubMsg->Buffer, sizeof(double)); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(double)); + copy_to_buffer(pStubMsg, pMemory, sizeof(double)); break; case FC_HYPER: align_pointer_clear(&pStubMsg->Buffer, sizeof(ULONGLONG)); - safe_copy_to_buffer(pStubMsg, pMemory, sizeof(ULONGLONG)); + copy_to_buffer(pStubMsg, pMemory, sizeof(ULONGLONG)); TRACE("value: %s\n", wine_dbgstr_longlong(*(ULONGLONG*)pMemory)); break; case FC_ENUM16: @@ -6664,7 +6656,7 @@ static unsigned char *WINAPI NdrBaseTypeMarshall( if (*(UINT *)pMemory > SHRT_MAX) RpcRaiseException(RPC_X_ENUM_VALUE_OUT_OF_RANGE); align_pointer_clear(&pStubMsg->Buffer, sizeof(USHORT)); - safe_copy_to_buffer(pStubMsg, &val, sizeof(val)); + copy_to_buffer(pStubMsg, &val, sizeof(val)); TRACE("value: 0x%04x\n", *(UINT *)pMemory); break; } @@ -6673,7 +6665,7 @@ static unsigned char *WINAPI NdrBaseTypeMarshall( { UINT val = *(UINT_PTR *)pMemory; align_pointer_clear(&pStubMsg->Buffer, sizeof(UINT)); - safe_copy_to_buffer(pStubMsg, &val, sizeof(val)); + copy_to_buffer(pStubMsg, &val, sizeof(val)); break; } case FC_IGNORE: diff --git a/dlls/rpcrt4/tests/ndr_marshall.c b/dlls/rpcrt4/tests/ndr_marshall.c index 0281dd241f6..fc21c5346f2 100644 --- a/dlls/rpcrt4/tests/ndr_marshall.c +++ b/dlls/rpcrt4/tests/ndr_marshall.c @@ -2999,6 +2999,38 @@ static void test_NdrCorrelationInitialize(void) ok( stub_msg.CorrDespIncrement == 1, "got %d\n", stub_msg.CorrDespIncrement ); } +static void test_pointer_validation(void) +{ + MIDL_STUB_DESC desc = Object_StubDesc; + RPC_MESSAGE rpc_msg; + MIDL_STUB_MESSAGE msg; + LONG l = 1; + unsigned char buf[32]; + + static const unsigned char format[] = + { + 0x12, 0x8, /* FC_UP [simple_pointer] */ + 0x8, /* FC_LONG */ + 0x5c, /* FC_PAD */ + }; + + desc.pFormatTypes = format; + NdrClientInitializeNew( &rpc_msg, &msg, &desc, 0 ); + + /* Pass invalid buffer pointers to check that they are not validated */ + msg.RpcMsg->Buffer = msg.BufferStart = (void *)0xdeadbeef; + + msg.BufferLength = 1; + msg.Buffer = buf; + NdrPointerMarshall(&msg, (unsigned char*)&l, format); + ok(msg.Buffer == buf + 8, "%p %p\n", msg.Buffer, msg.BufferStart); + + msg.BufferLength = ~0u; + msg.Buffer = buf; + NdrPointerMarshall(&msg, (unsigned char*)&l, format); + ok(msg.Buffer == buf + 8, "%p %p\n", msg.Buffer, msg.BufferStart); +} + START_TEST( ndr_marshall ) { determine_pointer_marshalling_style(); @@ -3023,4 +3055,5 @@ START_TEST( ndr_marshall ) test_NdrGetUserMarshalInfo(); test_MesEncodeFixedBufferHandleCreate(); test_NdrCorrelationInitialize(); + test_pointer_validation(); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7357
Looking closer at this, Windows doesn't appear to do any validation in this case. I updated with a patch dropping it for the marshaler, with more to follow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7357#note_96119
participants (2)
-
Jacek Caban -
Jacek Caban (＠jacek)