> On 5 May 2023, at 17:23, Anthony PERARD <anthony.per...@citrix.com> wrote:
>
> On Tue, May 02, 2023 at 07:54:19PM +0000, Luca Fancellu wrote:
>>> On 2 May 2023, at 18:06, Anthony PERARD <anthony.per...@citrix.com> wrote:
>>> On Mon, Apr 24, 2023 at 07:02:46AM +0100, Luca Fancellu wrote:
>>>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>>>> index ddc7b2a15975..1e69dac2c4fa 100644
>>>> --- a/tools/libs/light/libxl_arm.c
>>>> +++ b/tools/libs/light/libxl_arm.c
>>>> @@ -211,6 +213,12 @@ int libxl__arch_domain_prepare_config(libxl__gc *gc,
>>>> return ERROR_FAIL;
>>>> }
>>>>
>>>> + /* Parameter is sanitised in libxl__arch_domain_build_info_setdefault
>>>> */
>>>> + if (d_config->b_info.arch_arm.sve_vl) {
>>>> + /* Vector length is divided by 128 in struct
>>>> xen_domctl_createdomain */
>>>> + config->arch.sve_vl = d_config->b_info.arch_arm.sve_vl / 128U;
>>>> + }
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> @@ -1681,6 +1689,26 @@ int
>>>> libxl__arch_domain_build_info_setdefault(libxl__gc *gc,
>>>> /* ACPI is disabled by default */
>>>> libxl_defbool_setdefault(&b_info->acpi, false);
>>>>
>>>> + /* Sanitise SVE parameter */
>>>> + if (b_info->arch_arm.sve_vl) {
>>>> + unsigned int max_sve_vl =
>>>> + arch_capabilities_arm_sve(physinfo->arch_capabilities);
>>>> +
>>>> + if (!max_sve_vl) {
>>>> + LOG(ERROR, "SVE is unsupported on this machine.");
>>>> + return ERROR_FAIL;
>>>> + }
>>>> +
>>>> + if (LIBXL_SVE_TYPE_HW == b_info->arch_arm.sve_vl) {
>>>> + b_info->arch_arm.sve_vl = max_sve_vl;
>>>> + } else if (b_info->arch_arm.sve_vl > max_sve_vl) {
>>>> + LOG(ERROR,
>>>> + "Invalid sve value: %d. Platform supports up to %u bits",
>>>> + b_info->arch_arm.sve_vl, max_sve_vl);
>>>> + return ERROR_FAIL;
>>>> + }
>>>
>>> You still need to check that sve_vl is one of the value from the enum,
>>> or that the value is divisible by 128.
>>
>> I have probably missed something, I thought that using the way below to
>> specify the input I had for free that the value is 0 or divisible by 128, is
>> it
>> not the case? Who can write to b_info->arch_arm.sve_vl different value
>> from the enum we specified in the .idl?
>
> `xl` isn't the only user of `libxl`. There's `libvirt` as well. We also
> have libxl bindings for several languages.
Right, this point wasn’t clear to me, I will add the check there, thank you
for the explanation.
> There's nothing stopping a
> developer to write a number into `sve_vl` instead of choosing one of the
> value from the enum. I think we should probably sanitize any input that
> comes from outside of libxl, that's probably the case, I'm not sure.
>
> So if valid values for `sve_vl` are only numbers divisible by 128, and
> some other discrete numbers, then we should check that they are, or
> check that the value is one defined by the enum.
>
> Cheers,
>
> --
> Anthony PERARD
