|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] Add check for maximum xenstore key lengths
Hi,
On 19/08/2025 12:52, Owen Smith wrote:
> Fail any xenstore call that exceeds the maximum lengths, as defined
> in xs_wire.h
>
> Signed-off-by: Owen Smith <owen.smith@xxxxxxxxx>
> ---
> src/xenbus/store.c | 159 ++++++++++++++++++++++++++++++++-------------
> 1 file changed, 114 insertions(+), 45 deletions(-)
>
> diff --git a/src/xenbus/store.c b/src/xenbus/store.c
> index afb1954..1ce77ed 100644
> --- a/src/xenbus/store.c
> +++ b/src/xenbus/store.c
> @@ -1099,6 +1099,34 @@ RtlCaptureStackBackTrace(
> __out_opt PULONG BackTraceHash
> );
>
> +static NTSTATUS
> +StoreCheckPathLength(
> + __in_opt PSTR Prefix,
> + __in PSTR Node
> + )
Could you replace the annotations with SAL2 ones (_In_, _In_opt_)?
> +{
> + ULONG Length;
> + ULONG MaximumLength;
> +
> + if (Prefix != NULL) {
> + MaximumLength = Prefix[0] == '/' ?
If Prefix is an empty string, the resulting path will be of the form
"/Node". This check should take this case into account and use
XENSTORE_ABS_PATH_MAX instead.
> + XENSTORE_ABS_PATH_MAX :
> + XENSTORE_REL_PATH_MAX;
> + Length = (ULONG)strlen(Prefix) + 1;
> + } else {
> + MaximumLength = Node[0] == '/' ?
> + XENSTORE_ABS_PATH_MAX :
> + XENSTORE_REL_PATH_MAX;
> + Length = 0;
> + }
> +
> + Length += (ULONG)strlen(Node) + 1;
I think you should check for overflow of Length, both due to the
downcast to ULONG and the addition.
Also, is the +1 necessary here? I don't know if XENSTORE_*_PATH_MAX
counts the null terminator.
> +
> + return Length > MaximumLength ?
> + STATUS_INVALID_BUFFER_SIZE :
> + STATUS_SUCCESS;
> +}
> +
> static NTSTATUS
> StoreRead(
> _In_ PINTERFACE Interface,
> @@ -1116,6 +1144,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 +1177,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 +1202,15 @@ StoreRead(
>
> return STATUS_SUCCESS;
>
> +fail5:
> fail4:
> -fail3:
> StoreFreeResponse(Response);
>
> +fail3:
> fail2:
> -fail1:
> ASSERT(IsZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST)));
>
> +fail1:
> return status;
> }
>
> @@ -1195,6 +1228,10 @@ StoreWrite(
> PXENBUS_STORE_RESPONSE Response;
> NTSTATUS status;
>
> + status = StoreCheckPathLength(Prefix, Node);
> + if (!NT_SUCCESS(status))
> + goto fail1;
> +
StoreWrite is indirectly called by StoreVPrintf, and eventually
StorePrintf which already checks for this, so another check here
shouldn't be necessary. I think checking at the interface boundaries
should be enough unless the path is further transformed.
> RtlZeroMemory(&Request, sizeof (XENBUS_STORE_REQUEST));
>
> KeAcquireSpinLock(&Context->Lock, &Irql);
> @@ -1224,30 +1261,31 @@ StoreWrite(
> 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;
> }
>
> @@ -1326,6 +1364,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 +1378,9 @@ StorePrintf(
> va_end(Arguments);
>
> return status;
> +
> +fail1:
> + return status;
> }
>
> static NTSTATUS
> @@ -1352,6 +1397,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 +1428,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 +1473,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 +1506,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 +1535,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 +1727,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 +1749,7 @@ StoreWatchAdd(
>
> status = STATUS_NO_MEMORY;
> if (Path == NULL)
> - goto fail2;
> + goto fail3;
>
> status = (Prefix == NULL) ?
> RtlStringCbPrintfA(Path, Length, "%s", Node) :
> @@ -1736,24 +1795,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 +1829,8 @@ fail3:
>
> __StoreFree(Path);
>
> -fail2:
> - Error("fail2\n");
> +fail3:
> + Error("fail3\n");
>
> (*Watch)->Caller = NULL;
> (*Watch)->Magic = 0;
> @@ -1779,6 +1838,9 @@ fail2:
> ASSERT(IsZeroMemory(*Watch, sizeof (XENBUS_STORE_WATCH)));
> __StoreFree(*Watch);
>
> +fail2:
> + Error("fail2\n");
> +
> fail1:
> Error("fail1 (%08x)\n", status);
>
> @@ -2057,11 +2119,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 +2137,7 @@ StorePermissionsSet(
> Path = __StoreAllocate(Length);
>
> if (Path == NULL)
> - goto fail2;
> + goto fail3;
>
> status = (Prefix == NULL) ?
> RtlStringCbPrintfA(Path, Length, "%s", Node) :
> @@ -2088,7 +2154,7 @@ StorePermissionsSet(
> Length,
> &Used);
> if (!NT_SUCCESS(status))
> - goto fail3;
> + goto fail4;
>
> Segment += Used;
> Length -= Used;
> @@ -2108,17 +2174,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 +2194,12 @@ StorePermissionsSet(
>
> return STATUS_SUCCESS;
>
> +fail7:
> + Error("fail7\n");
> + StoreFreeResponse(Response);
> +
> fail6:
> Error("fail6\n");
> - StoreFreeResponse(Response);
>
> fail5:
> Error("fail5\n");
> @@ -2138,17 +2207,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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |