|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] Add check for maximum xenstore key lengths
On 21/08/2025 21:59, Tu Dinh wrote:
> On 21/08/2025 12:00, Owen Smith wrote:
>> Check any xenstore path lengths before calling into xenstore, in order
>> to fail without passing control to xenstored, which should fail any request
>> with paths exceeding the defined maximum length.
>>
>> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
>> ---
>> src/xenbus/store.c | 149 ++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 108 insertions(+), 41 deletions(-)
>>
>> diff --git a/src/xenbus/store.c b/src/xenbus/store.c
>> index afb1954..0c00d23 100644
>> --- a/src/xenbus/store.c
>> +++ b/src/xenbus/store.c
>> @@ -1099,6 +1099,37 @@ RtlCaptureStackBackTrace(
>> __out_opt PULONG BackTraceHash
>> );
>>
>> +static NTSTATUS
>> +StoreCheckPathLength(
>> + _In_opt_ PSTR Prefix,
>> + _In_ PSTR Node
>> + )
>> +{
>> + size_t Length;
>> + size_t MaximumLength;
>
> size_t will still overflow on 32-bit systems. I think you can keep
> things as ULONG and just have a preliminary check for
> XENSTORE_PAYLOAD_MAX on both strlen(Prefix) and strlen(Node), which will
> also take care of the overflow checking.
>
My bad, the lengths can't be ULONG but must remain size_t.
>> +
>> + Length = Prefix == NULL ? 0 : strlen(Prefix);
>> +
>> + if (Length == 0) {
>> + // Prefix is NULL or empty
>> + MaximumLength = Node[0] == '/' ?
>> + XENSTORE_ABS_PATH_MAX :
>> + XENSTORE_REL_PATH_MAX;
>> +
>> + Length = strlen(Node) + 1;
>> + } else {
>> + MaximumLength = Prefix[0] == '/' ?
>> + XENSTORE_ABS_PATH_MAX :
>> + XENSTORE_REL_PATH_MAX;
>> +
>> + Length += 1 + strlen(Node) + 1;
>> + }
>> +
>> + return Length > MaximumLength ?
>> + STATUS_INVALID_BUFFER_SIZE :
>> + STATUS_SUCCESS;
>> +}
>> +
>> static NTSTATUS
>> StoreRead(
>> _In_ PINTERFACE Interface,
>> @@ -1116,6 +1147,10 @@ StoreRead(
>> PXENBUS_STORE_BUFFER Buffer;
>> NTSTATUS status;
>>
>> + status = StoreCheckPathLength(Prefix, Node);
>> + if (!NT_SUCCESS(status))
>> + goto fail1;
>> +
>> (VOID) RtlCaptureStackBackTrace(1, 1, &Caller, NULL);
>>
>> RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
>> @@ -1145,23 +1180,23 @@ StoreRead(
>> KeReleaseSpinLock(&Context->Lock, Irql);
>>
>> if (!NT_SUCCESS(status))
>> - goto fail1;
>> + goto fail2;
>>
>> Response = StoreSubmitRequest(Context, &Request);
>>
>> status = STATUS_NO_MEMORY;
>> if (Response == NULL)
>> - goto fail2;
>> + goto fail3;
>>
>> status = StoreCheckResponse(Response);
>> if (!NT_SUCCESS(status))
>> - goto fail3;
>> + goto fail4;
>>
>> Buffer = StoreCopyPayload(Context, Response, Caller);
>>
>> status = STATUS_NO_MEMORY;
>> if (Buffer == NULL)
>> - goto fail4;
>> + goto fail5;
>>
>> StoreFreeResponse(Response);
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>> @@ -1170,14 +1205,15 @@ StoreRead(
>>
>> return STATUS_SUCCESS;
>>
>> +fail5:
>> fail4:
>> -fail3:
>> StoreFreeResponse(Response);
>>
>> +fail3:
>> fail2:
>> -fail1:
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> +fail1:
>> return status;
>> }
>>
>> @@ -1245,9 +1281,9 @@ fail3:
>> StoreFreeResponse(Response);
>>
>> fail2:
>> -fail1:
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> +fail1:
>> return status;
>> }
>>
>> @@ -1326,6 +1362,10 @@ StorePrintf(
>> va_list Arguments;
>> NTSTATUS status;
>>
>> + status = StoreCheckPathLength(Prefix, Node);
>> + if (!NT_SUCCESS(status))
>> + goto fail1;
>> +
>> va_start(Arguments, Format);
>> status = StoreVPrintf(Interface,
>> Transaction,
>> @@ -1336,6 +1376,9 @@ StorePrintf(
>> va_end(Arguments);
>>
>> return status;
>> +
>> +fail1:
>> + return status;
>> }
>>
>> static NTSTATUS
>> @@ -1352,6 +1395,10 @@ StoreRemove(
>> PXENBUS_STORE_RESPONSE Response;
>> NTSTATUS status;
>>
>> + status = StoreCheckPathLength(Prefix, Node);
>> + if (!NT_SUCCESS(status))
>> + goto fail1;
>> +
>> RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
>>
>> KeAcquireSpinLock(&Context->Lock, &Irql);
>> @@ -1379,30 +1426,31 @@ StoreRemove(
>> KeReleaseSpinLock(&Context->Lock, Irql);
>>
>> if (!NT_SUCCESS(status))
>> - goto fail1;
>> + goto fail2;
>>
>> Response = StoreSubmitRequest(Context, &Request);
>>
>> status = STATUS_NO_MEMORY;
>> if (Response == NULL)
>> - goto fail2;
>> + goto fail3;
>>
>> status = StoreCheckResponse(Response);
>> if (!NT_SUCCESS(status))
>> - goto fail3;
>> + goto fail4;
>>
>> StoreFreeResponse(Response);
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> return STATUS_SUCCESS;
>>
>> -fail3:
>> +fail4:
>> StoreFreeResponse(Response);
>>
>> +fail3:
>> fail2:
>> -fail1:
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> +fail1:
>> return status;
>> }
>>
>> @@ -1423,6 +1471,10 @@ StoreDirectory(
>> PXENBUS_STORE_BUFFER Buffer;
>> NTSTATUS status;
>>
>> + status = StoreCheckPathLength(Prefix, Node);
>> + if (!NT_SUCCESS(status))
>> + goto fail1;
>> +
>> (VOID) RtlCaptureStackBackTrace(1, 1, &Caller, NULL);
>>
>> RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
>> @@ -1452,27 +1504,27 @@ StoreDirectory(
>> KeReleaseSpinLock(&Context->Lock, Irql);
>>
>> if (!NT_SUCCESS(status))
>> - goto fail1;
>> + goto fail2;
>>
>> Response = StoreSubmitRequest(Context, &Request);
>>
>> status = STATUS_NO_MEMORY;
>> if (Response == NULL)
>> - goto fail2;
>> + goto fail3;
>>
>> status = StoreCheckResponse(Response);
>> if (!NT_SUCCESS(status))
>> - goto fail3;
>> + goto fail4;
>>
>> Buffer = StoreCopyPayload(Context, Response, Caller);
>>
>> status = STATUS_NO_MEMORY;
>> if (Buffer == NULL)
>> - goto fail4;
>> + goto fail5;
>>
>> status = STATUS_OBJECT_PATH_NOT_FOUND;
>> if (Buffer->Length == 0)
>> - goto fail5;
>> + goto fail6;
>>
>> StoreFreeResponse(Response);
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>> @@ -1481,17 +1533,18 @@ StoreDirectory(
>>
>> return STATUS_SUCCESS;
>>
>> -fail5:
>> +fail6:
>> StoreFreePayload(Context, Buffer);
>>
>> +fail5:
>> fail4:
>> -fail3:
>> StoreFreeResponse(Response);
>>
>> +fail3:
>> fail2:
>> -fail1:
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> +fail1:
>> return status;
>> }
>>
>> @@ -1672,11 +1725,15 @@ StoreWatchAdd(
>> KIRQL Irql;
>> NTSTATUS status;
>>
>> + status = StoreCheckPathLength(Prefix, Node);
>> + if (!NT_SUCCESS(status))
>> + goto fail1;
>> +
>> *Watch = __StoreAllocate(sizeof (XENBUS_STORE_WATCH));
>>
>> status = STATUS_NO_MEMORY;
>> if (*Watch == NULL)
>> - goto fail1;
>> + goto fail2;
>>
>> (*Watch)->Magic = STORE_WATCH_MAGIC;
>> (VOID) RtlCaptureStackBackTrace(1, 1, &(*Watch)->Caller, NULL);
>> @@ -1690,7 +1747,7 @@ StoreWatchAdd(
>>
>> status = STATUS_NO_MEMORY;
>> if (Path == NULL)
>> - goto fail2;
>> + goto fail3;
>>
>> status = (Prefix == NULL) ?
>> RtlStringCbPrintfA(Path, Length, "%s", Node) :
>> @@ -1736,24 +1793,24 @@ StoreWatchAdd(
>>
>> status = STATUS_NO_MEMORY;
>> if (Response == NULL)
>> - goto fail3;
>> + goto fail4;
>>
>> status = StoreCheckResponse(Response);
>> if (!NT_SUCCESS(status))
>> - goto fail4;
>> + goto fail5;
>>
>> StoreFreeResponse(Response);
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> return STATUS_SUCCESS;
>>
>> -fail4:
>> - Error("fail4\n");
>> +fail5:
>> + Error("fail5\n");
>>
>> StoreFreeResponse(Response);
>>
>> -fail3:
>> - Error("fail3\n");
>> +fail4:
>> + Error("fail4\n");
>>
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>>
>> @@ -1770,8 +1827,8 @@ fail3:
>>
>> __StoreFree(Path);
>>
>> -fail2:
>> - Error("fail2\n");
>> +fail3:
>> + Error("fail3\n");
>>
>> (*Watch)->Caller = NULL;
>> (*Watch)->Magic = 0;
>> @@ -1779,6 +1836,9 @@ fail2:
>> ASSERT(IsZeroMemory(*Watch, sizeof (XENBUS_STORE_WATCH)));
>> __StoreFree(*Watch);
>>
>> +fail2:
>> + Error("fail2\n");
>> +
>> fail1:
>> Error("fail1 (%08x)\n", status);
>>
>> @@ -2057,11 +2117,15 @@ StorePermissionsSet(
>> PSTR PermissionString;
>> PSTR Segment;
>>
>> + status = StoreCheckPathLength(Prefix, Node);
>> + if (!NT_SUCCESS(status))
>> + goto fail1;
>> +
>> PermissionString = __StoreAllocate(XENSTORE_PAYLOAD_MAX);
>>
>> status = STATUS_NO_MEMORY;
>> if (PermissionString == NULL)
>> - goto fail1;
>> + goto fail2;
>>
>> if (Prefix == NULL)
>> Length = (ULONG)strlen(Node) + sizeof (CHAR);
>> @@ -2071,7 +2135,7 @@ StorePermissionsSet(
>> Path = __StoreAllocate(Length);
>>
>> if (Path == NULL)
>> - goto fail2;
>> + goto fail3;
>>
>> status = (Prefix == NULL) ?
>> RtlStringCbPrintfA(Path, Length, "%s", Node) :
>> @@ -2088,7 +2152,7 @@ StorePermissionsSet(
>> Length,
>> &Used);
>> if (!NT_SUCCESS(status))
>> - goto fail3;
>> + goto fail4;
>>
>> Segment += Used;
>> Length -= Used;
>> @@ -2108,17 +2172,17 @@ StorePermissionsSet(
>> KeReleaseSpinLock(&Context->Lock, Irql);
>>
>> if (!NT_SUCCESS(status))
>> - goto fail4;
>> + goto fail5;
>>
>> Response = StoreSubmitRequest(Context, &Request);
>>
>> status = STATUS_NO_MEMORY;
>> if (Response == NULL)
>> - goto fail5;
>> + goto fail6;
>>
>> status = StoreCheckResponse(Response);
>> if (!NT_SUCCESS(status))
>> - goto fail6;
>> + goto fail7;
>>
>> StoreFreeResponse(Response);
>> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>> @@ -2128,9 +2192,12 @@ StorePermissionsSet(
>>
>> return STATUS_SUCCESS;
>>
>> +fail7:
>> + Error("fail7\n");
>> + StoreFreeResponse(Response);
>> +
>> fail6:
>> Error("fail6\n");
>> - StoreFreeResponse(Response);
>>
>> fail5:
>> Error("fail5\n");
>> @@ -2138,17 +2205,17 @@ fail5:
>> fail4:
>> Error("fail4\n");
>>
>> + __StoreFree(Path);
>> + ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>> +
>> fail3:
>> Error("fail3\n");
>>
>> - __StoreFree(Path);
>> - ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>> + __StoreFree(PermissionString);
>>
>> fail2:
>> Error("fail2\n");
>>
>> - __StoreFree(PermissionString);
>> -
>> fail1:
>> Error("fail1 (%08x)\n", status);
>>
>
>
>
> Ngoc Tu Dinh | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
>
>
Ngoc Tu Dinh | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |