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

[Mounah Honore Cesaire <cesaire.mounah@xxxxxxxxxx>]

On 10/22/26 1:39 PM, Tu Dinh wrote:
> 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.

Indeed. Let me fix this.

>
>> +    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");
>
>


--
Césaire Mounah | Vates Windows Guest Tools Engineer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech