diff mbox

[v13,19/24] selftests/vm: associate key on a mapped page and detect access violation

Message ID 1528937115-10132-20-git-send-email-linuxram@us.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ram Pai June 14, 2018, 12:45 a.m. UTC 
detect access-violation on a page to which access-disabled
key is associated much after the page is mapped.

cc: Dave Hansen <dave.hansen@intel.com>
cc: Florian Weimer <fweimer@redhat.com>
Signed-off-by: Ram Pai <linuxram@us.ibm.com>
Acked-by: Dave Hansen <dave.hansen@intel.com>
---
 tools/testing/selftests/vm/protection_keys.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

Comments

Dave Hansen June 20, 2018, 3:16 p.m. UTC | #1 
On 06/13/2018 05:45 PM, Ram Pai wrote:
> +void test_read_of_access_disabled_region_with_page_already_mapped(int *ptr,
> +		u16 pkey)
> +{
> +	int ptr_contents;
> +
> +	dprintf1("disabling access to PKEY[%02d], doing read @ %p\n",
> +				pkey, ptr);
> +	ptr_contents = read_ptr(ptr);
> +	dprintf1("reading ptr before disabling the read : %d\n",
> +			ptr_contents);
> +	read_pkey_reg();
> +	pkey_access_deny(pkey);
> +	ptr_contents = read_ptr(ptr);
> +	dprintf1("*ptr: %d\n", ptr_contents);
> +	expected_pkey_fault(pkey);
> +}

Looks fine to me.  I'm a bit surprised we didn't do this already, which
is a good thing for this patch.

FWIW, if you took patches like this and put them first, you could
probably get it merged now.  Yes, I know it would mean redoing some of
the later code move and rename ones.
Ram Pai July 17, 2018, 4:13 p.m. UTC | #2 
On Wed, Jun 20, 2018 at 08:16:44AM -0700, Dave Hansen wrote:
> On 06/13/2018 05:45 PM, Ram Pai wrote:
> > +void test_read_of_access_disabled_region_with_page_already_mapped(int *ptr,
> > +		u16 pkey)
> > +{
> > +	int ptr_contents;
> > +
> > +	dprintf1("disabling access to PKEY[%02d], doing read @ %p\n",
> > +				pkey, ptr);
> > +	ptr_contents = read_ptr(ptr);
> > +	dprintf1("reading ptr before disabling the read : %d\n",
> > +			ptr_contents);
> > +	read_pkey_reg();
> > +	pkey_access_deny(pkey);
> > +	ptr_contents = read_ptr(ptr);
> > +	dprintf1("*ptr: %d\n", ptr_contents);
> > +	expected_pkey_fault(pkey);
> > +}
> 
> Looks fine to me.  I'm a bit surprised we didn't do this already, which
> is a good thing for this patch.
> 
> FWIW, if you took patches like this and put them first, you could
> probably get it merged now.  Yes, I know it would mean redoing some of
> the later code move and rename ones.

I have incorporated almost all of your comments. But there are some
comments that take some effort to implement. Shall we get the patches
merged in the current form?  This code has been sitting out for a while.

In the current form its tested and works on powerpc and on x86, and
incorporates about 95% of your suggestions. The rest I will take care
as we go.
Dave Hansen July 17, 2018, 5:56 p.m. UTC | #3 
On 07/17/2018 09:13 AM, Ram Pai wrote:
> I have incorporated almost all of your comments. But there are some
> comments that take some effort to implement. Shall we get the patches
> merged in the current form?  This code has been sitting out for a while.
> 
> In the current form its tested and works on powerpc and on x86, and
> incorporates about 95% of your suggestions. The rest I will take care
> as we go.

What constitutes the remaining 5%?
Ram Pai July 17, 2018, 7:10 p.m. UTC | #4 
On Tue, Jul 17, 2018 at 10:56:08AM -0700, Dave Hansen wrote:
> On 07/17/2018 09:13 AM, Ram Pai wrote:
> > I have incorporated almost all of your comments. But there are some
> > comments that take some effort to implement. Shall we get the patches
> > merged in the current form?  This code has been sitting out for a while.
> > 
> > In the current form its tested and works on powerpc and on x86, and
> > incorporates about 95% of your suggestions. The rest I will take care
> > as we go.
> 
> What constitutes the remaining 5%?

Mostly your comments on code-organization in the signal handler. There
are still some #if defined(__i386__)  ..... Can be cleaned up and
abstracted further.

Also your questions on some of the code changes, the rationale for which
is not obvious. Will help to spinkle in some descriptive comments there.

Have fixed up a lot of codying style issues. But there could till be a
few that may spew warning by checkpatch.pl.

There are no functional issues AFAICT.

RP
diff mbox

Patch

diff --git a/tools/testing/selftests/vm/protection_keys.c b/tools/testing/selftests/vm/protection_keys.c
index e8ad970..04d0249 100644
--- a/tools/testing/selftests/vm/protection_keys.c
+++ b/tools/testing/selftests/vm/protection_keys.c
@@ -1024,6 +1024,24 @@  void test_read_of_access_disabled_region(int *ptr, u16 pkey)
 	dprintf1("*ptr: %d\n", ptr_contents);
 	expected_pkey_fault(pkey);
 }
+
+void test_read_of_access_disabled_region_with_page_already_mapped(int *ptr,
+		u16 pkey)
+{
+	int ptr_contents;
+
+	dprintf1("disabling access to PKEY[%02d], doing read @ %p\n",
+				pkey, ptr);
+	ptr_contents = read_ptr(ptr);
+	dprintf1("reading ptr before disabling the read : %d\n",
+			ptr_contents);
+	read_pkey_reg();
+	pkey_access_deny(pkey);
+	ptr_contents = read_ptr(ptr);
+	dprintf1("*ptr: %d\n", ptr_contents);
+	expected_pkey_fault(pkey);
+}
+
 void test_write_of_write_disabled_region(int *ptr, u16 pkey)
 {
 	dprintf1("disabling write access to PKEY[%02d], doing write\n", pkey);
@@ -1402,6 +1420,7 @@  void test_mprotect_pkey_on_unsupported_cpu(int *ptr, u16 pkey)
 void (*pkey_tests[])(int *ptr, u16 pkey) = {
 	test_read_of_write_disabled_region,
 	test_read_of_access_disabled_region,
+	test_read_of_access_disabled_region_with_page_already_mapped,
 	test_write_of_write_disabled_region,
 	test_write_of_access_disabled_region,
 	test_kernel_write_of_access_disabled_region,