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

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

On 03/11/2021 16:00, 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 v3 -> v4:
>    - add missing "Context" in the comments.  [Andra]
>    - move the comment in ne_merge_phys_contig_memory_regions() before
>      the "if (...)". [Andra]
> 
> 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 | 77 +++++++++++++++++++++----------
>   1 file changed, 52 insertions(+), 25 deletions(-)

Reviewed-by: Andra Paraschiv <andraprs@amazon.com>

You can add in the v5 of the patch series this line after the 
"Signed-off-by" line, to track the already reviewed patches.

Thanks,
Andra

> 
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index b7d2116..3741ae7 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -836,6 +836,37 @@ 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.
> + *
> + * Context: Process context. This function is called with the ne_enclave mutex held.
> + * Return:
> + * * 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 physically contiguous.
>    * @phys_contig_regions : Private data associated with the contiguous physical memory regions.
> @@ -843,23 +874,31 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>    * @page_size :           Length of the region to be added.
>    *
>    * Context: Process context. This function is called with the ne_enclave mutex held.
> + * Return:
> + * * 0 on success.
> + * * Negative return value on failure.
>    */
> -static void
> +static int
>   ne_merge_phys_contig_memory_regions(struct ne_phys_contig_mem_regions *phys_contig_regions,
>                                      u64 page_paddr, u64 page_size)
>   {
>          unsigned long num = phys_contig_regions->num;
> +       int rc = 0;
> +
> +       rc = ne_sanity_check_phys_mem_region(page_paddr, page_size);
> +       if (rc < 0)
> +               return rc;
> 
>          /* Physically contiguous, just merge */
>          if (num && (phys_contig_regions->regions[num - 1].end + 1) == page_paddr) {
>                  phys_contig_regions->regions[num - 1].end += page_size;
> -
> -               return;
> +       } else {
> +               phys_contig_regions->regions[num].start = page_paddr;
> +               phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
> +               phys_contig_regions->num++;
>          }
> 
> -       phys_contig_regions->regions[num].start = page_paddr;
> -       phys_contig_regions->regions[num].end = page_paddr + page_size - 1;
> -       phys_contig_regions->num++;
> +       return 0;
>   }
> 
>   /**
> @@ -938,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]);
> 
> @@ -962,23 +1003,9 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  u64 phys_region_addr = phys_contig_mem_regions.regions[i].start;
>                  u64 phys_region_size = range_len(&phys_contig_mem_regions.regions[i]);
> 
> -               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.