Re: [PATCH v6 05/15] xen: extend domctl interface for cache coloring

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

On Tue, Feb 6, 2024 at 12:51 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 06.02.2024 12:46, Carlo Nonato wrote:
> > On Thu, Feb 1, 2024 at 2:51 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >> On 29.01.2024 18:18, Carlo Nonato wrote:
> >>> @@ -229,6 +230,30 @@ int __init dom0_set_llc_colors(struct domain *d)
> >>>      return domain_check_colors(d);
> >>>  }
> >>>
> >>> +int domain_set_llc_colors_domctl(struct domain *d,
> >>> +                                 const struct xen_domctl_set_llc_colors 
> >>> *config)
> >>
> >> What purpose has the "domctl" in the function name?
> >>
> >>> +{
> >>> +    unsigned int *colors;
> >>> +
> >>> +    if ( d->num_llc_colors )
> >>> +        return -EEXIST;
> >>> +
> >>> +    if ( !config->num_llc_colors )
> >>> +        return domain_set_default_colors(d);
> >>> +
> >>> +    colors = alloc_colors(config->num_llc_colors);
> >>> +    if ( !colors )
> >>> +        return -ENOMEM;
> >>
> >> Hmm, I see here you call the function without first having bounds checked
> >> the input. But in case of too big a value, -ENOMEM is inappropriate, so
> >> such a check wants adding up front anyway.
> >>
> >>> +    if ( copy_from_guest(colors, config->llc_colors, 
> >>> config->num_llc_colors) )
> >>> +        return -EFAULT;
> >>
> >> There again wants to be a check that the pointed to values are the same,
> >> or at least of the same size. Else you'd need to do element-wise copy.
> >
> > Sorry to bring this back again, but I've just noticed copy_from_guest()
> > already checks for type compatibility. For what regards the size I don't 
> > think
> > I understood what to check. colors is defined to be of the same size of
> > config->llc_colors.
>
> Oh, you're right. But the other case was a memcpy(), wasn't it?

Yes, in that case your suggestion was needed.

Thanks again.

> Jan