Re: [PATCHv3 5/5] xeniface: Bound NumberPermissions before checking the input length

[Tu Dinh <ngoc-tu.dinh@xxxxxxxxxx>]

On 22/10/2026 13:36, Césaire Mounah wrote:
> IoctlStoreSetPermissions() compares InLen with
> FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, Permissions[N]), whose
> 64-bit result is truncated to ULONG. The count was only capped to 255
> later, in __ConvertPermissions(), so the length check alone could be
> satisfied by a huge count.
> 
> Check the cap first, with the same STATUS_INVALID_PARAMETER as before.
> The check in __ConvertPermissions() is removed and fails are updated
> accordingly.
> 
> Signed-off-by: Césaire Mounah <cesaire.mounah@xxxxxxxxxx>
> ---
>   src/xeniface/ioctl_store.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/src/xeniface/ioctl_store.c b/src/xeniface/ioctl_store.c
> index 97bef75..b28b44d 100644
> --- a/src/xeniface/ioctl_store.c
> +++ b/src/xeniface/ioctl_store.c
> @@ -417,6 +417,11 @@ IoctlStoreSetPermissions(
>           goto fail1;
>       }
>   
> +    status = STATUS_INVALID_PARAMETER;
> +    if (In->NumberPermissions > 255)
> +        goto fail2;
> +

I think we should avoid reusing the fail2 error label.

> +    status = STATUS_INVALID_BUFFER_SIZE;
>       if (InLen != (ULONG)FIELD_OFFSET(XENIFACE_STORE_SET_PERMISSIONS_IN, 
> Permissions[In->NumberPermissions]))
>           goto fail2;
>   
> @@ -427,17 +432,15 @@ IoctlStoreSetPermissions(
>       }
>   
>       Permissions = __ConvertPermissions(In->NumberPermissions, 
> In->Permissions);
> -    if (Permissions == NULL)
> -        goto fail4;
>   
>       status = __CaptureUserBuffer(In->Path, In->PathLength, &Path);
>       if (!NT_SUCCESS(status))
> -        goto fail5;
> +        goto fail4;
>   
>       Path[In->PathLength - 1] = 0;
>       status = STATUS_INVALID_PARAMETER;
>       if (!__IsValidPath(Path, In->PathLength))
> -        goto fail6;
> +        goto fail5;
>   
>       Trace("> Path '%s', NumberPermissions %lu\n", Path, 
> In->NumberPermissions);
>   
> @@ -457,25 +460,22 @@ IoctlStoreSetPermissions(
>                             In->NumberPermissions);
>   
>       if (!NT_SUCCESS(status))
> -        goto fail7;
> +        goto fail6;
>   
>       __FreePermissions(Permissions);
>       __FreeCapturedBuffer(Path);
>       return status;
>   
> -fail7:
> -    Error("Fail7\n");
> -
>   fail6:
>       Error("Fail6\n");
> -    __FreeCapturedBuffer(Path);
>   
>   fail5:
>       Error("Fail5\n");
> -    __FreePermissions(Permissions);
> +    __FreeCapturedBuffer(Path);
>   
>   fail4:
>       Error("Fail4\n");
> +    __FreePermissions(Permissions);
>   
>   fail3:
>       Error("Fail3\n");



--
Ngoc Tu Dinh | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech