diff mbox series

[v8,3/5] selftests/sgx: Dump enclave memory map

Message ID 20210610083021.392269-3-jarkko@kernel.org (mailing list archive)
State Changes Requested
Headers show
Series [v8,1/5] selftests/sgx: Rename 'eenter' and 'sgx_call_vdso' | expand

Commit Message

Jarkko Sakkinen June 10, 2021, 8:30 a.m. UTC
Often, it's useful to check whether /proc/self/maps looks sane when
dealing with memory mapped objects, especially when they are JIT'ish
dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
matching lines from the memory map in FIXTURE_SETUP().

Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
---
 tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

Comments

Shuah Khan June 11, 2021, 10:45 p.m. UTC | #1
On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> Often, it's useful to check whether /proc/self/maps looks sane when
> dealing with memory mapped objects, especially when they are JIT'ish
> dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
> matching lines from the memory map in FIXTURE_SETUP().
> 
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
>   tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> index 6da19b6bf287..14030f8b85ff 100644
> --- a/tools/testing/selftests/sgx/main.c
> +++ b/tools/testing/selftests/sgx/main.c
> @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
>   	Elf64_Sym *sgx_enter_enclave_sym = NULL;
>   	struct vdso_symtab symtab;
>   	struct encl_segment *seg;
> +	char maps_line[256];
> +	FILE *maps_file;
>   	unsigned int i;
>   	void *addr;
>   
> @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
>   	memset(&self->run, 0, sizeof(self->run));
>   	self->run.tcs = self->encl.encl_base;
>   
> +	maps_file = fopen("/proc/self/maps", "r");

I almost applied these. Does this require root access, if so,
please add logic to skip the test if non-root user runs it.

Same comments for all other paths that might require root access.

thanks,
-- Shuah
Dave Hansen June 12, 2021, 12:34 a.m. UTC | #2
On 6/11/21 3:45 PM, Shuah Khan wrote:
>>
>>   @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
>>       memset(&self->run, 0, sizeof(self->run));
>>       self->run.tcs = self->encl.encl_base;
>>   +    maps_file = fopen("/proc/self/maps", "r");
> 
> I almost applied these. Does this require root access, if so,
> please add logic to skip the test if non-root user runs it.
> 
> Same comments for all other paths that might require root access.

This specifically doesn't require root.  Anything can read its own
/proc/self/maps file.
Jarkko Sakkinen June 12, 2021, 4:27 a.m. UTC | #3
On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
> On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > Often, it's useful to check whether /proc/self/maps looks sane when
> > dealing with memory mapped objects, especially when they are JIT'ish
> > dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
> > matching lines from the memory map in FIXTURE_SETUP().
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > ---
> >   tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > index 6da19b6bf287..14030f8b85ff 100644
> > --- a/tools/testing/selftests/sgx/main.c
> > +++ b/tools/testing/selftests/sgx/main.c
> > @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
> >   	Elf64_Sym *sgx_enter_enclave_sym = NULL;
> >   	struct vdso_symtab symtab;
> >   	struct encl_segment *seg;
> > +	char maps_line[256];
> > +	FILE *maps_file;
> >   	unsigned int i;
> >   	void *addr;
> > @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
> >   	memset(&self->run, 0, sizeof(self->run));
> >   	self->run.tcs = self->encl.encl_base;
> > +	maps_file = fopen("/proc/self/maps", "r");
> 
> I almost applied these. Does this require root access, if so,
> please add logic to skip the test if non-root user runs it.
> 
> Same comments for all other paths that might require root access.

As Dave stated, it does not. A process can inspect its own state
through /proc/self path. E.g. Chrome web browser uses /proc/self/exe
to initialize multiple instances of itself for browser tabs...

As far as other things go, this patch set does not bind to any other
new OS resources.

> thanks,
> -- Shuah

/Jarkko
Shuah Khan June 14, 2021, 4:45 p.m. UTC | #4
On 6/11/21 10:27 PM, Jarkko Sakkinen wrote:
> On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
>> On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
>>> Often, it's useful to check whether /proc/self/maps looks sane when
>>> dealing with memory mapped objects, especially when they are JIT'ish
>>> dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
>>> matching lines from the memory map in FIXTURE_SETUP().
>>>
>>> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
>>> ---
>>>    tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
>>>    1 file changed, 14 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
>>> index 6da19b6bf287..14030f8b85ff 100644
>>> --- a/tools/testing/selftests/sgx/main.c
>>> +++ b/tools/testing/selftests/sgx/main.c
>>> @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
>>>    	Elf64_Sym *sgx_enter_enclave_sym = NULL;
>>>    	struct vdso_symtab symtab;
>>>    	struct encl_segment *seg;
>>> +	char maps_line[256];
>>> +	FILE *maps_file;
>>>    	unsigned int i;
>>>    	void *addr;
>>> @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
>>>    	memset(&self->run, 0, sizeof(self->run));
>>>    	self->run.tcs = self->encl.encl_base;
>>> +	maps_file = fopen("/proc/self/maps", "r");
>>
>> I almost applied these. Does this require root access, if so,
>> please add logic to skip the test if non-root user runs it.
>>
>> Same comments for all other paths that might require root access.
> 
> As Dave stated, it does not. A process can inspect its own state
> through /proc/self path. E.g. Chrome web browser uses /proc/self/exe
> to initialize multiple instances of itself for browser tabs...
> 

Ah yes. I missed the self part. Thanks for clarifying.

> As far as other things go, this patch set does not bind to any other
> new OS resources.
> 

Thank you. I will apply these for 5.14-rc1.

thanks,
-- Shuah
Jarkko Sakkinen June 15, 2021, 1:07 p.m. UTC | #5
On Mon, Jun 14, 2021 at 10:45:43AM -0600, Shuah Khan wrote:
> On 6/11/21 10:27 PM, Jarkko Sakkinen wrote:
> > On Fri, Jun 11, 2021 at 04:45:19PM -0600, Shuah Khan wrote:
> > > On 6/10/21 2:30 AM, Jarkko Sakkinen wrote:
> > > > Often, it's useful to check whether /proc/self/maps looks sane when
> > > > dealing with memory mapped objects, especially when they are JIT'ish
> > > > dynamically constructed objects. Therefore, dump "/dev/sgx_enclave"
> > > > matching lines from the memory map in FIXTURE_SETUP().
> > > > 
> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> > > > ---
> > > >    tools/testing/selftests/sgx/main.c | 14 ++++++++++++++
> > > >    1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
> > > > index 6da19b6bf287..14030f8b85ff 100644
> > > > --- a/tools/testing/selftests/sgx/main.c
> > > > +++ b/tools/testing/selftests/sgx/main.c
> > > > @@ -117,6 +117,8 @@ FIXTURE_SETUP(enclave)
> > > >    	Elf64_Sym *sgx_enter_enclave_sym = NULL;
> > > >    	struct vdso_symtab symtab;
> > > >    	struct encl_segment *seg;
> > > > +	char maps_line[256];
> > > > +	FILE *maps_file;
> > > >    	unsigned int i;
> > > >    	void *addr;
> > > > @@ -167,6 +169,18 @@ FIXTURE_SETUP(enclave)
> > > >    	memset(&self->run, 0, sizeof(self->run));
> > > >    	self->run.tcs = self->encl.encl_base;
> > > > +	maps_file = fopen("/proc/self/maps", "r");
> > > 
> > > I almost applied these. Does this require root access, if so,
> > > please add logic to skip the test if non-root user runs it.
> > > 
> > > Same comments for all other paths that might require root access.
> > 
> > As Dave stated, it does not. A process can inspect its own state
> > through /proc/self path. E.g. Chrome web browser uses /proc/self/exe
> > to initialize multiple instances of itself for browser tabs...
> > 
> 
> Ah yes. I missed the self part. Thanks for clarifying.
> 
> > As far as other things go, this patch set does not bind to any other
> > new OS resources.
> > 
> 
> Thank you. I will apply these for 5.14-rc1.

Thank you!

> thanks,
> -- Shuah

/Jarkko
diff mbox series

Patch

diff --git a/tools/testing/selftests/sgx/main.c b/tools/testing/selftests/sgx/main.c
index 6da19b6bf287..14030f8b85ff 100644
--- a/tools/testing/selftests/sgx/main.c
+++ b/tools/testing/selftests/sgx/main.c
@@ -117,6 +117,8 @@  FIXTURE_SETUP(enclave)
 	Elf64_Sym *sgx_enter_enclave_sym = NULL;
 	struct vdso_symtab symtab;
 	struct encl_segment *seg;
+	char maps_line[256];
+	FILE *maps_file;
 	unsigned int i;
 	void *addr;
 
@@ -167,6 +169,18 @@  FIXTURE_SETUP(enclave)
 	memset(&self->run, 0, sizeof(self->run));
 	self->run.tcs = self->encl.encl_base;
 
+	maps_file = fopen("/proc/self/maps", "r");
+	if (maps_file != NULL)  {
+		while (fgets(maps_line, sizeof(maps_line), maps_file) != NULL) {
+			maps_line[strlen(maps_line) - 1] = '\0';
+
+			if (strstr(maps_line, "/dev/sgx_enclave"))
+				TH_LOG("%s", maps_line);
+		}
+
+		fclose(maps_file);
+	}
+
 err:
 	if (!sgx_enter_enclave_sym)
 		encl_delete(&self->encl);