diff mbox series

[v2,1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR

Message ID 20200403093529.43587-1-npiggin@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2,1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR | expand

Checks

Context Check Description
snowpatch_ozlabs/apply_patch success Successfully applied on branch powerpc/merge (d0c12846a3a24cd6d68b608c866712bc7e471634)
snowpatch_ozlabs/checkpatch warning total: 0 errors, 1 warnings, 0 checks, 95 lines checked
snowpatch_ozlabs/needsstable success Patch has no Fixes tags

Commit Message

Nicholas Piggin April 3, 2020, 9:35 a.m. UTC 
There is no need to allow user accesses when probing kernel addresses.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
v2:
- Enable for all powerpc (suggested by Christophe)
- Fold helper function together (Christophe)
- Rename uaccess.c to maccess.c to match kernel/maccess.c.

 arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
 arch/powerpc/lib/Makefile          |  2 +-
 arch/powerpc/lib/maccess.c         | 34 ++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 9 deletions(-)
 create mode 100644 arch/powerpc/lib/maccess.c

Comments

Christophe Leroy April 3, 2020, 10:31 a.m. UTC | #1 
Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.

I just discovered the following commit 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4

This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().

When reading the commit log, I understand that probe_kernel_read() may 
be used to access some user memory. Which will not work anymore with 
your patch.

Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that 
you want to add ?

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2:
> - Enable for all powerpc (suggested by Christophe)
> - Fold helper function together (Christophe)
> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
> 
>   arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
>   arch/powerpc/lib/Makefile          |  2 +-
>   arch/powerpc/lib/maccess.c         | 34 ++++++++++++++++++++++++++++++

x86 does it in mm/maccess.c

>   3 files changed, 52 insertions(+), 9 deletions(-)
>   create mode 100644 arch/powerpc/lib/maccess.c
> 

Christophe
Nicholas Piggin April 3, 2020, 11:05 a.m. UTC | #2 
Christophe Leroy's on April 3, 2020 8:31 pm:
> 
> 
> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
> 
> I just discovered the following commit 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
> 
> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
> 
> When reading the commit log, I understand that probe_kernel_read() may 
> be used to access some user memory. Which will not work anymore with 
> your patch.

Hmm, I looked at _strict but obviously not hard enough. Good catch.

I don't think probe_kernel_read() should ever access user memory,
the comment certainly says it doesn't, but that patch sort of implies
that they do.

I think it's wrong. The non-_strict maybe could return userspace data to 
you if you did pass a user address? I don't see why that shouldn't just 
be disallowed always though.

And if the _strict version is required to be safe, then it seems like a
bug or security issue to just allow everyone that doesn't explicitly
override it to use the default implementation.

Also, the way the weak linkage is done in that patch, means parisc and
um archs that were previously overriding probe_kernel_read() now get
the default probe_kernel_read_strict(), which would be wrong for them.

> 
> Isn't it probe_kernel_read_strict() and probe_kernel_write_strict() that 
> you want to add ?
> 
>> 
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> v2:
>> - Enable for all powerpc (suggested by Christophe)
>> - Fold helper function together (Christophe)
>> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
>> 
>>   arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
>>   arch/powerpc/lib/Makefile          |  2 +-
>>   arch/powerpc/lib/maccess.c         | 34 ++++++++++++++++++++++++++++++
> 
> x86 does it in mm/maccess.c

Yeah I'll fix that up, thanks.

Thanks,
Nick
Nicholas Piggin April 7, 2020, 4:01 a.m. UTC | #3 
Nicholas Piggin's on April 3, 2020 9:05 pm:
> Christophe Leroy's on April 3, 2020 8:31 pm:
>> 
>> 
>> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>>> There is no need to allow user accesses when probing kernel addresses.
>> 
>> I just discovered the following commit 
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
>> 
>> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
>> 
>> When reading the commit log, I understand that probe_kernel_read() may 
>> be used to access some user memory. Which will not work anymore with 
>> your patch.
> 
> Hmm, I looked at _strict but obviously not hard enough. Good catch.
> 
> I don't think probe_kernel_read() should ever access user memory,
> the comment certainly says it doesn't, but that patch sort of implies
> that they do.
> 
> I think it's wrong. The non-_strict maybe could return userspace data to 
> you if you did pass a user address? I don't see why that shouldn't just 
> be disallowed always though.
> 
> And if the _strict version is required to be safe, then it seems like a
> bug or security issue to just allow everyone that doesn't explicitly
> override it to use the default implementation.
> 
> Also, the way the weak linkage is done in that patch, means parisc and
> um archs that were previously overriding probe_kernel_read() now get
> the default probe_kernel_read_strict(), which would be wrong for them.

The changelog in commit 75a1a607bb7 makes it a bit clearer. If the
non-_strict variant is used on non-kernel addresses, then it might not 
return -EFAULT or it might cause a kernel warning. The _strict variant 
is supposed to be usable with any address and it will return -EFAULT if 
it was not a valid and mapped kernel address.

The non-_strict variant can not portably access user memory because it
uses KERNEL_DS, and its documentation says its only for kernel pointers.
So powerpc should be fine to run that under KUAP AFAIKS.

I don't know why the _strict behaviour is not just made default, but
the implementation of it does seem to be broken on the archs that
override the non-_strict variant.

Thanks,
Nick
Daniel Borkmann April 7, 2020, 9:09 a.m. UTC | #4 
Hey Nicholas,

On 4/7/20 6:01 AM, Nicholas Piggin wrote:
> Nicholas Piggin's on April 3, 2020 9:05 pm:
>> Christophe Leroy's on April 3, 2020 8:31 pm:
>>> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>>>> There is no need to allow user accesses when probing kernel addresses.
>>>
>>> I just discovered the following commit
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=75a1a607bb7e6d918be3aca11ec2214a275392f4
>>>
>>> This commit adds probe_kernel_read_strict() and probe_kernel_write_strict().
>>>
>>> When reading the commit log, I understand that probe_kernel_read() may
>>> be used to access some user memory. Which will not work anymore with
>>> your patch.
>>
>> Hmm, I looked at _strict but obviously not hard enough. Good catch.
>>
>> I don't think probe_kernel_read() should ever access user memory,
>> the comment certainly says it doesn't, but that patch sort of implies
>> that they do.
>>
>> I think it's wrong. The non-_strict maybe could return userspace data to
>> you if you did pass a user address? I don't see why that shouldn't just
>> be disallowed always though.
>>
>> And if the _strict version is required to be safe, then it seems like a
>> bug or security issue to just allow everyone that doesn't explicitly
>> override it to use the default implementation.
>>
>> Also, the way the weak linkage is done in that patch, means parisc and
>> um archs that were previously overriding probe_kernel_read() now get
>> the default probe_kernel_read_strict(), which would be wrong for them.
> 
> The changelog in commit 75a1a607bb7 makes it a bit clearer. If the
> non-_strict variant is used on non-kernel addresses, then it might not
> return -EFAULT or it might cause a kernel warning. The _strict variant
> is supposed to be usable with any address and it will return -EFAULT if
> it was not a valid and mapped kernel address.
> 
> The non-_strict variant can not portably access user memory because it
> uses KERNEL_DS, and its documentation says its only for kernel pointers.
> So powerpc should be fine to run that under KUAP AFAIKS.
> 
> I don't know why the _strict behaviour is not just made default, but
> the implementation of it does seem to be broken on the archs that
> override the non-_strict variant.

Yeah, we should make it default and only add a "opt out" for the old legacy
cases; there was also same discussion started over here just recently [0].

Thanks,
Daniel

   [0] https://lore.kernel.org/lkml/20200403133533.GA3424@infradead.org/T/
Christophe Leroy June 10, 2020, 12:41 p.m. UTC | #5 
Hi Nick

Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
> There is no need to allow user accesses when probing kernel addresses.

You should have a look at 
https://github.com/torvalds/linux/commit/fa94111d94354de76c47fea6e1187d1ee91e23a7

At seems to implement a generic way of achieving what you are trying to 
do here.

Christophe

> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
> v2:
> - Enable for all powerpc (suggested by Christophe)
> - Fold helper function together (Christophe)
> - Rename uaccess.c to maccess.c to match kernel/maccess.c.
> 
>   arch/powerpc/include/asm/uaccess.h | 25 +++++++++++++++-------
>   arch/powerpc/lib/Makefile          |  2 +-
>   arch/powerpc/lib/maccess.c         | 34 ++++++++++++++++++++++++++++++
>   3 files changed, 52 insertions(+), 9 deletions(-)
>   create mode 100644 arch/powerpc/lib/maccess.c
> 
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 2f500debae21..670910df3cc7 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -341,8 +341,8 @@ raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
>   }
>   #endif /* __powerpc64__ */
>   
> -static inline unsigned long raw_copy_from_user(void *to,
> -		const void __user *from, unsigned long n)
> +static inline unsigned long
> +raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
>   {
>   	unsigned long ret;
>   	if (__builtin_constant_p(n) && (n <= 8)) {
> @@ -351,19 +351,19 @@ static inline unsigned long raw_copy_from_user(void *to,
>   		switch (n) {
>   		case 1:
>   			barrier_nospec();
> -			__get_user_size(*(u8 *)to, from, 1, ret);
> +			__get_user_size_allowed(*(u8 *)to, from, 1, ret);
>   			break;
>   		case 2:
>   			barrier_nospec();
> -			__get_user_size(*(u16 *)to, from, 2, ret);
> +			__get_user_size_allowed(*(u16 *)to, from, 2, ret);
>   			break;
>   		case 4:
>   			barrier_nospec();
> -			__get_user_size(*(u32 *)to, from, 4, ret);
> +			__get_user_size_allowed(*(u32 *)to, from, 4, ret);
>   			break;
>   		case 8:
>   			barrier_nospec();
> -			__get_user_size(*(u64 *)to, from, 8, ret);
> +			__get_user_size_allowed(*(u64 *)to, from, 8, ret);
>   			break;
>   		}
>   		if (ret == 0)
> @@ -371,9 +371,18 @@ static inline unsigned long raw_copy_from_user(void *to,
>   	}
>   
>   	barrier_nospec();
> -	allow_read_from_user(from, n);
>   	ret = __copy_tofrom_user((__force void __user *)to, from, n);
> -	prevent_read_from_user(from, n);
> +	return ret;
> +}
> +
> +static inline unsigned long
> +raw_copy_from_user(void *to, const void __user *from, unsigned long n)
> +{
> +	unsigned long ret;
> +
> +	allow_read_from_user(to, n);
> +	ret = raw_copy_from_user_allowed(to, from, n);
> +	prevent_read_from_user(to, n);
>   	return ret;
>   }
>   
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index b8de3be10eb4..77af10ad0b0d 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
>   CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
>   endif
>   
> -obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
> +obj-y += alloc.o code-patching.o feature-fixups.o pmem.o maccess.o
>   
>   ifndef CONFIG_KASAN
>   obj-y	+=	string.o memcmp_$(BITS).o
> diff --git a/arch/powerpc/lib/maccess.c b/arch/powerpc/lib/maccess.c
> new file mode 100644
> index 000000000000..ce5465db1e2d
> --- /dev/null
> +++ b/arch/powerpc/lib/maccess.c
> @@ -0,0 +1,34 @@
> +#include <linux/mm.h>
> +#include <linux/uaccess.h>
> +
> +/*
> + * Override the generic weak linkage functions to avoid changing KUP state via
> + * the generic user access functions, as this is accessing kernel addresses.
> + */
> +long probe_kernel_read(void *dst, const void *src, size_t size)
> +{
> +	long ret;
> +	mm_segment_t old_fs = get_fs();
> +
> +	set_fs(KERNEL_DS);
> +	pagefault_disable();
> +	ret = raw_copy_from_user_allowed(dst, (__force const void __user *)src, size);
> +	pagefault_enable();
> +	set_fs(old_fs);
> +
> +	return ret ? -EFAULT : 0;
> +}
> +
> +long probe_kernel_write(void *dst, const void *src, size_t size)
> +{
> +	long ret;
> +	mm_segment_t old_fs = get_fs();
> +
> +	set_fs(KERNEL_DS);
> +	pagefault_disable();
> +	ret = raw_copy_to_user_allowed((__force void __user *)dst, src, size);
> +	pagefault_enable();
> +	set_fs(old_fs);
> +
> +	return ret ? -EFAULT : 0;
> +}
>
Nicholas Piggin June 14, 2020, 9:28 a.m. UTC | #6 
Excerpts from Christophe Leroy's message of June 10, 2020 10:41 pm:
> Hi Nick
> 
> Le 03/04/2020 à 11:35, Nicholas Piggin a écrit :
>> There is no need to allow user accesses when probing kernel addresses.
> 
> You should have a look at 
> https://github.com/torvalds/linux/commit/fa94111d94354de76c47fea6e1187d1ee91e23a7
> 
> At seems to implement a generic way of achieving what you are trying to 
> do here.

Yep thanks for that, I'll rebase this series on upstream now.

Thanks,
Nick
diff mbox series

Patch

diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
index 2f500debae21..670910df3cc7 100644
--- a/arch/powerpc/include/asm/uaccess.h
+++ b/arch/powerpc/include/asm/uaccess.h
@@ -341,8 +341,8 @@  raw_copy_in_user(void __user *to, const void __user *from, unsigned long n)
 }
 #endif /* __powerpc64__ */
 
-static inline unsigned long raw_copy_from_user(void *to,
-		const void __user *from, unsigned long n)
+static inline unsigned long
+raw_copy_from_user_allowed(void *to, const void __user *from, unsigned long n)
 {
 	unsigned long ret;
 	if (__builtin_constant_p(n) && (n <= 8)) {
@@ -351,19 +351,19 @@  static inline unsigned long raw_copy_from_user(void *to,
 		switch (n) {
 		case 1:
 			barrier_nospec();
-			__get_user_size(*(u8 *)to, from, 1, ret);
+			__get_user_size_allowed(*(u8 *)to, from, 1, ret);
 			break;
 		case 2:
 			barrier_nospec();
-			__get_user_size(*(u16 *)to, from, 2, ret);
+			__get_user_size_allowed(*(u16 *)to, from, 2, ret);
 			break;
 		case 4:
 			barrier_nospec();
-			__get_user_size(*(u32 *)to, from, 4, ret);
+			__get_user_size_allowed(*(u32 *)to, from, 4, ret);
 			break;
 		case 8:
 			barrier_nospec();
-			__get_user_size(*(u64 *)to, from, 8, ret);
+			__get_user_size_allowed(*(u64 *)to, from, 8, ret);
 			break;
 		}
 		if (ret == 0)
@@ -371,9 +371,18 @@  static inline unsigned long raw_copy_from_user(void *to,
 	}
 
 	barrier_nospec();
-	allow_read_from_user(from, n);
 	ret = __copy_tofrom_user((__force void __user *)to, from, n);
-	prevent_read_from_user(from, n);
+	return ret;
+}
+
+static inline unsigned long
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+	unsigned long ret;
+
+	allow_read_from_user(to, n);
+	ret = raw_copy_from_user_allowed(to, from, n);
+	prevent_read_from_user(to, n);
 	return ret;
 }
 
diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index b8de3be10eb4..77af10ad0b0d 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -16,7 +16,7 @@  CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
 CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
 endif
 
-obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
+obj-y += alloc.o code-patching.o feature-fixups.o pmem.o maccess.o
 
 ifndef CONFIG_KASAN
 obj-y	+=	string.o memcmp_$(BITS).o
diff --git a/arch/powerpc/lib/maccess.c b/arch/powerpc/lib/maccess.c
new file mode 100644
index 000000000000..ce5465db1e2d
--- /dev/null
+++ b/arch/powerpc/lib/maccess.c
@@ -0,0 +1,34 @@ 
+#include <linux/mm.h>
+#include <linux/uaccess.h>
+
+/*
+ * Override the generic weak linkage functions to avoid changing KUP state via
+ * the generic user access functions, as this is accessing kernel addresses.
+ */
+long probe_kernel_read(void *dst, const void *src, size_t size)
+{
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	pagefault_disable();
+	ret = raw_copy_from_user_allowed(dst, (__force const void __user *)src, size);
+	pagefault_enable();
+	set_fs(old_fs);
+
+	return ret ? -EFAULT : 0;
+}
+
+long probe_kernel_write(void *dst, const void *src, size_t size)
+{
+	long ret;
+	mm_segment_t old_fs = get_fs();
+
+	set_fs(KERNEL_DS);
+	pagefault_disable();
+	ret = raw_copy_to_user_allowed((__force void __user *)dst, src, size);
+	pagefault_enable();
+	set_fs(old_fs);
+
+	return ret ? -EFAULT : 0;
+}