Re: [PATCH v6 10/12] xen/tools: add sve parameter in XL configuration

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Tue, May 02, 2023 at 07:54:19PM +0000, Luca Fancellu wrote:
> > On 2 May 2023, at 18:06, Anthony PERARD <anthony.perard@xxxxxxxxxx> 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. 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