Re: [PATCH v7 05/14] xen: extend domctl interface for cache coloring

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Tue, Mar 19, 2024 at 4:37 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 15.03.2024 11:58, Carlo Nonato wrote:
> > @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
> >      xfree(__va(__pa(d->llc_colors)));
> >  }
> >
> > +int domain_set_llc_colors(struct domain *d,
> > +                          const struct xen_domctl_set_llc_colors *config)
> > +{
> > +    unsigned int *colors;
> > +
> > +    if ( d->num_llc_colors )
> > +        return -EEXIST;
> > +
> > +    if ( !config->num_llc_colors )
> > +        return domain_set_default_colors(d);
> > +
> > +    if ( config->num_llc_colors > max_nr_colors || config->pad )
>
> The check of "pad" wants carrying out in all cases; I expect it wants
> moving to the caller.

Ok.

> > +        return -EINVAL;
> > +
> > +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(colors, config->llc_colors, 
> > config->num_llc_colors) )
> > +        return -EFAULT;
>
> You're leaking "colors" when taking this or ...
>
> > +    if ( !check_colors(colors, config->num_llc_colors) )
> > +    {
> > +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> > +        return -EINVAL;
>
> ... this error path.

You're right.

> Jan

Thanks.

On Tue, Mar 19, 2024 at 4:37 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 15.03.2024 11:58, Carlo Nonato wrote:
> > @@ -219,6 +220,39 @@ void domain_llc_coloring_free(struct domain *d)
> >      xfree(__va(__pa(d->llc_colors)));
> >  }
> >
> > +int domain_set_llc_colors(struct domain *d,
> > +                          const struct xen_domctl_set_llc_colors *config)
> > +{
> > +    unsigned int *colors;
> > +
> > +    if ( d->num_llc_colors )
> > +        return -EEXIST;
> > +
> > +    if ( !config->num_llc_colors )
> > +        return domain_set_default_colors(d);
> > +
> > +    if ( config->num_llc_colors > max_nr_colors || config->pad )
>
> The check of "pad" wants carrying out in all cases; I expect it wants
> moving to the caller.
>
> > +        return -EINVAL;
> > +
> > +    colors = xmalloc_array(unsigned int, config->num_llc_colors);
> > +    if ( !colors )
> > +        return -ENOMEM;
> > +
> > +    if ( copy_from_guest(colors, config->llc_colors, 
> > config->num_llc_colors) )
> > +        return -EFAULT;
>
> You're leaking "colors" when taking this or ...
>
> > +    if ( !check_colors(colors, config->num_llc_colors) )
> > +    {
> > +        printk(XENLOG_ERR "Bad LLC color config for %pd\n", d);
> > +        return -EINVAL;
>
> ... this error path.
>
> Jan