Re: [PATCH v2 2/4] nitro_enclaves: sanity check the physical region during setting

[Paraschiv, Andra-Irina]

On 21/09/2021 18:10, Longpeng(Mike) wrote:
> Sanity check the physical region before add it to the array, this makes
> the code more testable, thus we can test the physical region setup logic
> individually.

Could have the following for the title:

nitro_enclaves: Sanity check physical memory regions during merging

Then the commit message body could be:

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(Mike) <longpeng2@xxxxxxxxxx>
> ---
>   drivers/virt/nitro_enclaves/ne_misc_dev.c | 62 +++++++++++++++++--------------
>   1 file changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index a4776fc..d551b88 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -844,10 +844,28 @@ static int ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
>          return 0;
>   }
>
> -static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
> -                                     u64 paddr, u64 size)
> +static inline int ne_sanity_check_phys_mem_region(u64 paddr, u64 size)

Please add a comment including a brief description for the function, the 
parameters and the return value, as there are available for the already 
existing functions. Also could be more specific and mention 
"phys_mem_region_paddr" and "phys_mem_region_size" for the parameters.

No need for "inline" here, there are a couple of sanity checks, e.g. not 
a very simple, one line logic.

> +{
> +       if (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(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;
> +}
> +
> +static int ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
> +                                    u64 paddr, u64 size)
>   {
>          u64 prev_phys_region_end = 0;
> +       int rc = 0;
>
>          if (regions->num) {
>                  prev_phys_region_end = regions->region[regions->num - 1].paddr +
> @@ -855,14 +873,23 @@ static void ne_add_phys_memory_region(struct phys_contig_mem_region *regions,
>
>                  /* Physical contiguous, just merge */
>                  if (prev_phys_region_end == paddr) {
> +                       rc = ne_sanity_check_phys_mem_region(paddr, size);
> +                       if (rc < 0)
> +                               return rc;
> +
>                          regions->region[regions->num - 1].size += size;
> -                       return;
> +                       return 0;

Can leave a blank line before return.

>                  }
>          }
>
> +       rc = ne_sanity_check_phys_mem_region(paddr, size);
> +       if (rc < 0)
> +               return rc;


This check can be moved at the beginning of the function, before the 
initial "if" code block. Then can remove the check from the 
"prev_phys_region_end" check block above.

> +
>          regions->region[regions->num].paddr = paddr;
>          regions->region[regions->num].size = size;
>          regions->num++;
> +       return 0;


Can leave a blank line before return.

>   }
>
>   /**
> @@ -942,8 +969,10 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  if (rc < 0)
>                          goto put_pages;
>
> -               ne_add_phys_memory_region(phys_regions, page_to_phys(ne_mem_region->pages[i]),
> -                                         page_size(ne_mem_region->pages[i]));
> +               rc = ne_add_phys_memory_region(phys_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]);
>
> @@ -960,29 +989,6 @@ static int ne_set_user_memory_region_ioctl(struct ne_enclave *ne_enclave,
>                  goto put_pages;
>          }
>
> -       for (i = 0; i < phys_regions->num; i++) {
> -               u64 phys_region_addr = phys_regions->region[i].paddr;
> -               u64 phys_region_size = phys_regions->region[i].size;
> -
> -               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;
> -
> -                       goto put_pages;
> -               }
> -       }
> -

Should also call the check function here, after the merge of physical 
contiguous memory regions has been completed, for double checking.

Thanks,
Andra

>          ne_mem_region->memory_size = mem_region.memory_size;
>          ne_mem_region->userspace_addr = mem_region.userspace_addr;
>
> --
> 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.