Re: [PATCH v5 2/8] xen/arm: allocate static shared memory to the default owner dom_io

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 04/07/2022 08:20, Penny Zheng wrote:
> > > > > +
> > > > > +    return acquire_static_memory_bank(d, NULL, addr_cells, size_cells,
> > > > > +                                      pbase, psize);
> > > > > +
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Func allocate_shared_memory is supposed to be only called
> > > > I am a bit concerned with the word "supposed". Are you implying that
> > > > it may be called by someone that is not the owner? If not, then it
> > > > should be "should".
> > > >
> > > > Also NIT: Spell out completely "func". I.e "The function".
> > > >
> > > > > + * from the owner.
> > > > I read from as "current should be the owner". But I guess this is not
> > > > what you mean here. Instead it looks like you mean "d" is the owner.
> > > > So I would write "d should be the owner of the shared area".
> > > >
> > > > It would be good to have a check/ASSERT confirm this (assuming this
> > > > is easy to write).
> > > The check is already in the calling path, I guess...
> > Can you please confirm it?
> I mean that allocate_shared_memory is only called under the following 
> condition, and
> it confirms it is the right owner.
> "
>          if ( (owner_dom_io && !is_shm_allocated_to_domio(pbase)) ||
>               (!owner_dom_io && strcmp(role_str, "owner") == 0) )
>          {
>              /* Allocate statically shared pages to the owner domain. */
>              ret = allocate_shared_memory(owner_dom_io ? dom_io : d,
>                                           addr_cells, size_cells,
>                                           pbase, psize, gbase);
> "
I agree that this looks to be the case today. But error can slip easily, 
so if we can add some ASSERT() in function then you could catch issues 
during development. Hence why I suggested to add an ASSERT() if possible.
> TBH, apart from that, I don't know how to check if the input d is the right 
> owner, or am I
> misunderstanding some your suggestion here?
Is page_get_owner() already properly set? If yes, you could ASSERT() the 
first page of the range belongs to 'd'.
This is more an hardening exercise, so it is not critical if it is 
difficult (or not possible) to have an ASSERT().
>   
> > [...]
> >
> > > > > +        prop = dt_find_property(shm_node, "xen,shared-mem", NULL);
> > > > > +        if ( !prop )
> > > > > +        {
> > > > > +            printk("Shared memory node does not provide
> > > > > + \"xen,shared-
> > > > mem\" property.\n");
> > > > > +            return -ENOENT;
> > > > > +        }
> > > > > +        cells = (const __be32 *)prop->value;
> > > > > +        /* xen,shared-mem = <pbase, psize, gbase>; */
> > > > > +        device_tree_get_reg(&cells, addr_cells, size_cells, &pbase, &psize);
> > > > > +        ASSERT(IS_ALIGNED(pbase, PAGE_SIZE) && IS_ALIGNED(psize,
> > > > > + PAGE_SIZE));
> > > > See above about what ASSERT()s are for.
> > > Do you think address was suitably checked here, is it enough?
> > As I wrote before, ASSERT() should not be used to check user inputs.
> > They need to happen in both debug and production build.
> >
> > > If it is enough, I'll modify above ASSERT() to mfn_valid()
> > It is not clear what ASSERT() you are referring to.
> For whether page is aligned, I will add the below check:
> "
>          if ( !IS_ALIGNED(pbase, PAGE_SIZE) || !IS_ALIGNED(psize, PAGE_SIZE) ||
>               !IS_ALIGNED(gbase, PAGE_SIZE) )
>          {
>              printk("%pd: physical address %lu, size %lu or guest address %lu is not 
> suitably aligned.\n",
AFAICT, the type for the 3 variables is paddr_t. So you want to use 
0x%"PRIpaddr" instead of %lu.
BTW, in general, for address it is preferable to use hexadecimal over 
decimal.
>                     d, pbase, psize, gbase);
>              return -EINVAL;
>          }
> "
> For whether the whole address range is valid, I will add the below check:
> "
>          for ( i = 0; i < PFN_DOWN(psize); i++ )
>              if ( !mfn_valid(mfn_add(maddr_to_mfn(pbase), i)) )
>              {
>                  printk("%pd: invalid physical address %"PRI_paddr" or size 
> %"PRI_paddr"\n",
s/PRI_paddr/PRIpaddr/

I am also not sure why you want to print the size here?

>                         d, pbase, psize);
>                  return -EINVAL;
>              }
> "
Cheers,

--
Julien Grall