| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 16 Sep 2021 02:26:35 +0000
From: "Longpeng (Mike, Cloud Infrastructure Service Product Dept.)"
<longpeng2@...wei.com>
To: "Paraschiv, Andra-Irina" <andraprs@...zon.com>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Gonglei (Arei)" <arei.gonglei@...wei.com>,
Greg KH <gregkh@...uxfoundation.org>,
Kamal Mostafa <kamal@...onical.com>,
Alexandru Vasile <lexnv@...zon.com>,
Alexandru Ciobotaru <alcioa@...zon.com>,
Paolo Bonzini <pbonzini@...hat.com>,
Stefano Garzarella <sgarzare@...hat.com>,
Stefan Hajnoczi <stefanha@...hat.com>,
Vitaly Kuznetsov <vkuznets@...hat.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"ne-devel-upstream@...zon.com" <ne-devel-upstream@...zon.com>
Subject: RE: [PATCH] nitro_enclaves: merge contiguous physical memory regions
> -----Original Message-----
> From: Paraschiv, Andra-Irina [mailto:andraprs@...zon.com]
> Sent: Thursday, September 16, 2021 1:47 AM
> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> <longpeng2@...wei.com>
> Cc: linux-kernel@...r.kernel.org; Gonglei (Arei) <arei.gonglei@...wei.com>; Greg
> KH <gregkh@...uxfoundation.org>; Kamal Mostafa <kamal@...onical.com>;
> Alexandru Vasile <lexnv@...zon.com>; Alexandru Ciobotaru
> <alcioa@...zon.com>; Paolo Bonzini <pbonzini@...hat.com>; Stefano Garzarella
> <sgarzare@...hat.com>; Stefan Hajnoczi <stefanha@...hat.com>; Vitaly
> Kuznetsov <vkuznets@...hat.com>; kvm@...r.kernel.org;
> ne-devel-upstream@...zon.com
> Subject: Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions
>
>
>
> On 15/09/2021 10:28, Longpeng (Mike, Cloud Infrastructure Service
> Product Dept.) wrote:
> >> -----Original Message-----
> >> From: Paraschiv, Andra-Irina [mailto:andraprs@...zon.com]
> >> Sent: Wednesday, September 15, 2021 1:45 AM
> >> To: Longpeng (Mike, Cloud Infrastructure Service Product Dept.)
> >> <longpeng2@...wei.com>
> >> Cc: linux-kernel@...r.kernel.org; Gonglei (Arei) <arei.gonglei@...wei.com>;
> Greg
> >> KH <gregkh@...uxfoundation.org>; Kamal Mostafa <kamal@...onical.com>;
> >> Alexandru Vasile <lexnv@...zon.com>; Alexandru Ciobotaru
> >> <alcioa@...zon.com>; Paolo Bonzini <pbonzini@...hat.com>; Stefano
> Garzarella
> >> <sgarzare@...hat.com>; Stefan Hajnoczi <stefanha@...hat.com>; Vitaly
> >> Kuznetsov <vkuznets@...hat.com>; kvm@...r.kernel.org;
> >> ne-devel-upstream@...zon.com
> >> Subject: Re: [PATCH] nitro_enclaves: merge contiguous physical memory regions
> >>
> >>
[snip]
> >> Then the logic from [2] can be updated to allocate the first part, the 2
> >> MiB memory regions, then the second part, the 8 MiB memory regions.
> >>
> >> Open for other options as well to cover the validation for the regions
> >> merging case.
> >>
> >
> > Okay, will do in the next version.
> >
> > By the way, I have another question here.
> >
> > The nitro_enclaves.ko consists of two parts: the misc part and the pci_dev part.
> > The main logic is in the misc part, the pci_dev part is responsible for the
> > device management and command routing.
> >
> > So how about to decouple these two parts, e.g. split the nitro_enclaves.ko into
> > ne_ioctl.ko and ne_pdev.ko ? In this way, we can allow other modules to
> connect
> > with ne_ioctl.ko:
> >
> > ne_ioctl.ko
> > ---/ | --\
> > ---/ | -----\
> > ---/ | ----\
> > ne_pdev.ko test_pdev.ko others...
> >
> >
> > With test_pdev.ko, we can validate the core logic of the ne_ioctl.ko in local.
> > What's more, other vendor's device could also support software enclave feature
> > if they follow the specification.
>
>
> During the discussions for the initial merge there has been specific
> feedback to make the misc functionality available only if the PCI device
> is present, as they are coming altogether. In addition, there are
> userspace tools, configurations, setups available in different
> environments, that rely on the current structure of the NE kernel
> driver, and changes would break the compatibility.
>
Got it, thanks :)
I have an alternative to support to validate the misc functionality in local, I'll
send it together in the next version.
> Thanks,
> Andra
>
>
> >
> >>>
> >>> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> >> b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> >>> index e21e1e8..2920f26 100644
> >>> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> >>> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> >>> @@ -824,6 +824,11 @@ static int
> >> ne_sanity_check_user_mem_region_page(struct ne_enclave *ne_enclave,
> >>> return 0;
> >>> }
> >>>
> >>> +struct phys_contig_mem_region {
> >>> + u64 paddr;
> >>> + u64 size;
> >>> +};
> >>> +
> >>> /**
> >>> * ne_set_user_memory_region_ioctl() - Add user space memory region to
> >> the slot
> >>> * associated with the current
> >> enclave.
> >>> @@ -843,9 +848,10 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>> unsigned long max_nr_pages = 0;
> >>> unsigned long memory_size = 0;
> >>> struct ne_mem_region *ne_mem_region = NULL;
> >>> - unsigned long nr_phys_contig_mem_regions = 0;
> >>> struct pci_dev *pdev = ne_devs.ne_pci_dev->pdev;
> >>> - struct page **phys_contig_mem_regions = NULL;
> >>> + struct phys_contig_mem_region *phys_regions = NULL;
> >>> + unsigned long nr_phys_regions = 0;
> >>> + u64 prev_phys_region_end;
> >>
> >> It's indeed initialized later on and it's not used till then, but let's
> >> just have an init to 0 here as well.
> >>
> >
> > Okay.
> >
> >>> int rc = -EINVAL;
> >>>
> >>> rc = ne_sanity_check_user_mem_region(ne_enclave,
> mem_region);
> >>> @@ -866,9 +872,8 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>> goto free_mem_region;
> >>> }
> >>>
> >>> - phys_contig_mem_regions = kcalloc(max_nr_pages,
> >> sizeof(*phys_contig_mem_regions),
> >>> - GFP_KERNEL);
> >>> - if (!phys_contig_mem_regions) {
> >>> + phys_regions = kcalloc(max_nr_pages, sizeof(*phys_regions),
> >> GFP_KERNEL);
> >>> + if (!phys_regions) {
> >>> rc = -ENOMEM;
> >>>
> >>> goto free_mem_region;
> >>> @@ -903,25 +908,29 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>>
> >>> /*
> >>> * TODO: Update once handled non-contiguous memory
> >> regions
> >>> - * received from user space or contiguous physical memory
> >> regions
> >>> - * larger than 2 MiB e.g. 8 MiB.
> >>> + * received from user space.
> >>> */
> >>
> >> Can remove this TODO completely, similar as below.
> >>
> >
> > Okay.
> >
> >>> - phys_contig_mem_regions[i] = ne_mem_region->pages[i];
> >>> + if (nr_phys_regions &&
> >>> + prev_phys_region_end ==
> >> page_to_phys(ne_mem_region->pages[i]))
> >>> + phys_regions[nr_phys_regions - 1].size +=
> >>> +
> >> page_size(ne_mem_region->pages[i]);
> >>
> >> Please add parantheses here as well.
> >>
> >> if ... {
> >>
> >>
> >> } else {
> >>
> >> }
> >>
> >>> + else {
> >>> + phys_regions[nr_phys_regions].paddr =
> >>> +
> >> page_to_phys(ne_mem_region->pages[i]);
> >>> + phys_regions[nr_phys_regions].size =
> >>> +
> >> page_size(ne_mem_region->pages[i]);
> >>> + nr_phys_regions++;
> >>> + }
> >>> +
> >>> + prev_phys_region_end = phys_regions[nr_phys_regions -
> >> 1].paddr +
> >>> +
> phys_regions[nr_phys_regions -
> >> 1].size;
> >>>
> >>> memory_size += page_size(ne_mem_region->pages[i]);
> >>>
> >>> ne_mem_region->nr_pages++;
> >>> } while (memory_size < mem_region.memory_size);
> >>>
> >>> - /*
> >>> - * TODO: Update once handled non-contiguous memory regions
> >> received
> >>> - * from user space or contiguous physical memory regions larger
> than
> >>> - * 2 MiB e.g. 8 MiB.
> >>> - */
> >>> - nr_phys_contig_mem_regions = ne_mem_region->nr_pages;
> >>> -
> >>> - if ((ne_enclave->nr_mem_regions + nr_phys_contig_mem_regions) >
> >>> - ne_enclave->max_mem_regions) {
> >>> + if ((ne_enclave->nr_mem_regions + nr_phys_regions) >
> >> ne_enclave->max_mem_regions) {
> >>> dev_err_ratelimited(ne_misc_dev.this_device,
> >>> "Reached max memory
> >> regions %lld\n",
> >>>
> ne_enclave->max_mem_regions);
> >>> @@ -931,11 +940,8 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>> goto put_pages;
> >>> }
> >>>
> >>> - for (i = 0; i < nr_phys_contig_mem_regions; i++) {
> >>> - u64 phys_region_addr =
> >> page_to_phys(phys_contig_mem_regions[i]);
> >>> - u64 phys_region_size =
> >> page_size(phys_contig_mem_regions[i]);
> >>> -
> >>> - if (phys_region_size & (NE_MIN_MEM_REGION_SIZE - 1)) {
> >>> + for (i = 0; i < nr_phys_regions; i++) {
> >>> + if (phys_regions[i].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");
> >>>
> >>> @@ -944,7 +950,7 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>> goto put_pages;
> >>> }
> >>>
> >>> - if (!IS_ALIGNED(phys_region_addr,
> >> NE_MIN_MEM_REGION_SIZE)) {
> >>> + if (!IS_ALIGNED(phys_regions[i].paddr,
> >> NE_MIN_MEM_REGION_SIZE)) {
> >>>
> dev_err_ratelimited(ne_misc_dev.this_device,
> >>> "Physical mem region
> >> address is not 2 MiB aligned\n");
> >>>
> >>> @@ -959,13 +965,13 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>>
> >>> list_add(&ne_mem_region->mem_region_list_entry,
> >> &ne_enclave->mem_regions_list);
> >>>
> >>> - for (i = 0; i < nr_phys_contig_mem_regions; i++) {
> >>> + for (i = 0; i < nr_phys_regions; i++) {
> >>> struct ne_pci_dev_cmd_reply cmd_reply = {};
> >>> struct slot_add_mem_req slot_add_mem_req = {};
> >>>
> >>> slot_add_mem_req.slot_uid = ne_enclave->slot_uid;
> >>> - slot_add_mem_req.paddr =
> >> page_to_phys(phys_contig_mem_regions[i]);
> >>> - slot_add_mem_req.size =
> >> page_size(phys_contig_mem_regions[i]);
> >>> + slot_add_mem_req.paddr = phys_regions[i].paddr;
> >>> + slot_add_mem_req.size = phys_regions[i].size;
> >>>
> >>> rc = ne_do_request(pdev, SLOT_ADD_MEM,
> >>> &slot_add_mem_req,
> >> sizeof(slot_add_mem_req),
> >>> @@ -974,7 +980,7 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>>
> dev_err_ratelimited(ne_misc_dev.this_device,
> >>> "Error in slot add
> mem
> >> [rc=%d]\n", rc);
> >>>
> >>> - kfree(phys_contig_mem_regions);
> >>> + kfree(phys_regions);
> >>>
> >>> /*
> >>> * Exit here without put pages as memory
> regions
> >> may
> >>> @@ -987,7 +993,7 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>> ne_enclave->nr_mem_regions++;
> >>> }
> >>>
> >>> - kfree(phys_contig_mem_regions);
> >>> + kfree(phys_regions);
> >>>
> >>> return 0;
> >>>
> >>> @@ -995,7 +1001,7 @@ static int ne_set_user_memory_region_ioctl(struct
> >> ne_enclave *ne_enclave,
> >>> for (i = 0; i < ne_mem_region->nr_pages; i++)
> >>> put_page(ne_mem_region->pages[i]);
> >>> free_mem_region:
> >>> - kfree(phys_contig_mem_regions);
> >>> + kfree(phys_regions);
> >>> kfree(ne_mem_region->pages);
> >>> kfree(ne_mem_region);
> >>>
> >>> --
> >>> 1.8.3.1
> >>>
> >>
> >> Please also add the changelog after the "---" line in the patches so
> >> that we can track changes between revisions.
> >>
> >
> > Got it, thanks.
> >
> >> Thanks,
> >> Andra
> >>
> >> [1]
> >>
> https://github.com/aws/aws-nitro-enclaves-cli/blob/main/src/enclave_proc/resour
> >> ce_manager.rs#L211
> >> [2]
> >>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/nitr
> >> o_enclaves/ne_ioctl_sample.c#n817
> >>
> >>
> >>
> >> 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.
>
>
>
> 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.
Powered by blists - more mailing lists