Re: [PATCH v3 2/4] nitro_enclaves: Sanity check physical memory regions during merging

["Paraschiv, Andra-Irina" <andraprs@amazon.com>]

On 09/10/2021 04:32, Longpeng(Mike) wrote:
> 
> From: Longpeng <longpeng2@huawei.com>
> 
> Sanity check the physical memory regions during the merge of contiguous
> regions. Thus we can test the physical memory regions setup logic
> individually, including the error cases coming from the sanity checks.
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
> Changes v2 -> v3:
>    - update the commit title and commit message.  [Andra]
>    - add comments before the function definition.  [Andra]
>    - remove 'inline' attribute of ne_sanity_check_phys_mem_region. [Andra]
>    - leave a blank line before return.  [Andra]
>    - move sanity check in ne_merge_phys_contig_memory_regions to
>      the beginning of the function.  [Andra]
>    - double sanity checking after the merge of physical contiguous
>      memory regions has been completed.  [Andra]
> ---
>   drivers/virt/nitro_enclaves/ne_misc_dev.c | 79 ++++++++++++++++++++-----------
>   1 file changed, 52 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index eea53e9..a8fa56b 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -836,6 +836,36 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>   }
> 
>   /**
> + * ne_sanity_check_phys_mem_region() - Sanity check the start address and the size
> + *                                     of a physical memory region.
> + * @phys_mem_region_paddr : Physical start address of the region to be sanity checked.
> + * @phys_mem_region_size  : Length of the region to be sanity checked.
> + *
> + * Return:

Please add this just before "Return", similar to other defined functions 
(e.g. [1]):

* Context: Process context. This function is called with the ne_enclave 
mutex held.
* Return:
......

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/virt/nitro_enclaves/ne_misc_dev.c#n725

> + * * 0 on success.
> + * * Negative return value on failure.
> + */
> +static int ne_sanity_check_phys_mem_region(u64 phys_mem_region_paddr,
> +                                          u64 phys_mem_region_size)
> +{
> +       if (phys_mem_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
> +               dev_err_ratelimited(ne_misc_dev.this_device,
> +                                   "Physical mem region size is not multiple of 2 MiB\n");
> +
> +               return -EINVAL;
> +       }
> +
> +       if (!IS_ALIGNED(phys_mem_region_paddr, NE_MIN_MEM_REGION_SIZE)) {
> +               dev_err_ratelimited(ne_misc_dev.this_device,
> +                                   "Physical mem region address is not 2 MiB aligned\n");
> +
> +               return -EINVAL;
> +       }
> +
> +       return 0;
> +}
> +
> +/**
>    * ne_merge_phys_contig_memory_regions() - Add a memory region and merge the adjacent
>    *                                         regions if they are physical contiguous.
>    * @regions   : Private data associated with the physical contiguous memory regions.
> @@ -843,23 +873,30 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>    * @page_size : Length of the region to be added.
>    *
>    * Return:
> - * * No return value.
> + * * 0 on success.
> + * * Negative return value on failure.
>    */
> -static void
> +static int
>   ne_merge_phys_contig_memory_regions(struct phys_contig_mem_regions *regions,
>                                      u64 page_paddr, u64 page_size)
>   {
> -       /* Physical contiguous, just merge */
> +       int rc = 0;
> +
> +       rc = ne_sanity_check_phys_mem_region(page_paddr, page_size);
> +       if (rc < 0)
> +               return rc;
> +
>          if (regions->num &&
>              (regions->region[regions->num - 1].end + 1) == page_paddr) {
> +               /* Physical contiguous, just merge */

Can move this comment before the "if (...)" line.

Thanks,
Andra

>                  regions->region[regions->num - 1].end += page_size;
> -
> -               return;
> +       } else {
> +               regions->region[regions->num].start = page_paddr;
> +               regions->region[regions->num].end = page_paddr + page_size - 1;
> +               regions->num++;
>          }
> 
> -       regions->region[regions->num].start = page_paddr;
> -       regions->region[regions->num].end = page_paddr + page_size - 1;
> -       regions->num++;
> +       return 0;
>   }
> 
>   /**
> @@ -940,9 +977,11 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  if (rc < 0)
>                          goto put_pages;
> 
> -               ne_merge_phys_contig_memory_regions(phys_contig_mem_regions,
> -                                                   page_to_phys(ne_mem_region->pages[i]),
> -                                                   page_size(ne_mem_region->pages[i]));
> +               rc = ne_merge_phys_contig_memory_regions(phys_contig_mem_regions,
> +                                                        page_to_phys(ne_mem_region->pages[i]),
> +                                                        page_size(ne_mem_region->pages[i]));
> +               if (rc < 0)
> +                       goto put_pages;
> 
>                  memory_size += page_size(ne_mem_region->pages[i]);
> 
> @@ -965,23 +1004,9 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  u64 phys_region_addr = range->start;
>                  u64 phys_region_size = range_len(range);
> 
> -               if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
> -                       dev_err_ratelimited(ne_misc_dev.this_device,
> -                                           "Physical mem region size is not multiple of 2 MiB\n");
> -
> -                       rc = -EINVAL;
> -
> -                       goto put_pages;
> -               }
> -
> -               if (!IS_ALIGNED(phys_region_addr, NE_MIN_MEM_REGION_SIZE)) {
> -                       dev_err_ratelimited(ne_misc_dev.this_device,
> -                                           "Physical mem region address is not 2 MiB aligned\n");
> -
> -                       rc = -EINVAL;
> -
> +               rc = ne_sanity_check_phys_mem_region(phys_region_addr, phys_region_size);
> +               if (rc < 0)
>                          goto put_pages;
> -               }
>          }
> 
>          ne_mem_region->memory_size = mem_region.memory_size;
> --
> 1.8.3.1
> 



Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.