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. 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
