arch/s390/include/asm/uaccess.h | 20 +++++++++++-- arch/s390/lib/uaccess.c | 50 +++++++++++++++++++-------------- include/linux/uaccess.h | 28 ++++++++++++++++++ 3 files changed, 75 insertions(+), 23 deletions(-)
KVM on s390 needs a mechanism to do accesses to guest memory
that honors storage key protection.
__copy_from/to_user_with_key is implemented by introducing
raw_copy_from/to_user_with_key.
Since the existing uaccess implementation on s390 makes use of move
instructions that support having an additional access key supplied,
we can implement raw_copy_from/to_user_with_key by enhancing the
existing implementation.
Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com>
---
This works for us and compiles on other architectures (tested x86).
The patch only implements __copy_from/to_user_with_key, since those
are the ones we actually need. On other architectures those functions
don't exists, but they aren't used either, so it's not a problem.
Should we also implement single and no underscore variants? Why?
Completeness?
arch/s390/include/asm/uaccess.h | 20 +++++++++++--
arch/s390/lib/uaccess.c | 50 +++++++++++++++++++--------------
include/linux/uaccess.h | 28 ++++++++++++++++++
3 files changed, 75 insertions(+), 23 deletions(-)
diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
index 02b467461163..fc8477b5e98c 100644
--- a/arch/s390/include/asm/uaccess.h
+++ b/arch/s390/include/asm/uaccess.h
@@ -33,11 +33,27 @@ static inline int __range_ok(unsigned long addr, unsigned long size)
#define access_ok(addr, size) __access_ok(addr, size)
+#define raw_copy_from_user_with_key raw_copy_from_user_with_key
unsigned long __must_check
-raw_copy_from_user(void *to, const void __user *from, unsigned long n);
+raw_copy_from_user_with_key(void *to, const void __user *from, unsigned long n,
+ unsigned long key);
+#define raw_copy_to_user_with_key raw_copy_to_user_with_key
unsigned long __must_check
-raw_copy_to_user(void __user *to, const void *from, unsigned long n);
+raw_copy_to_user_with_key(void __user *to, const void *from, unsigned long n,
+ unsigned long key);
+
+static __always_inline unsigned long __must_check
+raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+{
+ return raw_copy_from_user_with_key(to, from, n, 0);
+}
+
+static __always_inline unsigned long __must_check
+raw_copy_to_user(void __user *to, const void *from, unsigned long n)
+{
+ return raw_copy_to_user_with_key(to, from, n, 0);
+}
#ifndef CONFIG_KASAN
#define INLINE_COPY_FROM_USER
diff --git a/arch/s390/lib/uaccess.c b/arch/s390/lib/uaccess.c
index d3a700385875..63845dd82691 100644
--- a/arch/s390/lib/uaccess.c
+++ b/arch/s390/lib/uaccess.c
@@ -59,11 +59,13 @@ static inline int copy_with_mvcos(void)
#endif
static inline unsigned long copy_from_user_mvcos(void *x, const void __user *ptr,
- unsigned long size)
+ unsigned long size, u8 key)
{
unsigned long tmp1, tmp2;
union oac spec = {
+ .oac2.key = key,
.oac2.as = PSW_BITS_AS_SECONDARY,
+ .oac2.k = 1,
.oac2.a = 1,
};
@@ -94,19 +96,19 @@ static inline unsigned long copy_from_user_mvcos(void *x, const void __user *ptr
}
static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
- unsigned long size)
+ unsigned long size, u8 key)
{
unsigned long tmp1, tmp2;
tmp1 = -256UL;
asm volatile(
" sacf 0\n"
- "0: mvcp 0(%0,%2),0(%1),%3\n"
+ "0: mvcp 0(%0,%2),0(%1),%[key]\n"
"7: jz 5f\n"
"1: algr %0,%3\n"
" la %1,256(%1)\n"
" la %2,256(%2)\n"
- "2: mvcp 0(%0,%2),0(%1),%3\n"
+ "2: mvcp 0(%0,%2),0(%1),%[key]\n"
"8: jnz 1b\n"
" j 5f\n"
"3: la %4,255(%1)\n" /* %4 = ptr + 255 */
@@ -115,7 +117,7 @@ static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
" slgr %4,%1\n"
" clgr %0,%4\n" /* copy crosses next page boundary? */
" jnh 6f\n"
- "4: mvcp 0(%4,%2),0(%1),%3\n"
+ "4: mvcp 0(%4,%2),0(%1),%[key]\n"
"9: slgr %0,%4\n"
" j 6f\n"
"5: slgr %0,%0\n"
@@ -123,24 +125,28 @@ static inline unsigned long copy_from_user_mvcp(void *x, const void __user *ptr,
EX_TABLE(0b,3b) EX_TABLE(2b,3b) EX_TABLE(4b,6b)
EX_TABLE(7b,3b) EX_TABLE(8b,3b) EX_TABLE(9b,6b)
: "+a" (size), "+a" (ptr), "+a" (x), "+a" (tmp1), "=a" (tmp2)
- : : "cc", "memory");
+ : [key] "d" (key << 4)
+ : "cc", "memory");
return size;
}
-unsigned long raw_copy_from_user(void *to, const void __user *from, unsigned long n)
+unsigned long raw_copy_from_user_with_key(void *to, const void __user *from,
+ unsigned long n, unsigned long key)
{
if (copy_with_mvcos())
- return copy_from_user_mvcos(to, from, n);
- return copy_from_user_mvcp(to, from, n);
+ return copy_from_user_mvcos(to, from, n, (u8)key);
+ return copy_from_user_mvcp(to, from, n, (u8)key);
}
-EXPORT_SYMBOL(raw_copy_from_user);
+EXPORT_SYMBOL(raw_copy_from_user_with_key);
-static inline unsigned long copy_to_user_mvcos(void __user *ptr, const void *x,
- unsigned long size)
+inline unsigned long copy_to_user_mvcos(void __user *ptr, const void *x,
+ unsigned long size, u8 key)
{
unsigned long tmp1, tmp2;
union oac spec = {
+ .oac1.key = key,
.oac1.as = PSW_BITS_AS_SECONDARY,
+ .oac1.k = 1,
.oac1.a = 1,
};
@@ -171,19 +177,19 @@ static inline unsigned long copy_to_user_mvcos(void __user *ptr, const void *x,
}
static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
- unsigned long size)
+ unsigned long size, u8 key)
{
unsigned long tmp1, tmp2;
tmp1 = -256UL;
asm volatile(
" sacf 0\n"
- "0: mvcs 0(%0,%1),0(%2),%3\n"
+ "0: mvcs 0(%0,%1),0(%2),%[key]\n"
"7: jz 5f\n"
"1: algr %0,%3\n"
" la %1,256(%1)\n"
" la %2,256(%2)\n"
- "2: mvcs 0(%0,%1),0(%2),%3\n"
+ "2: mvcs 0(%0,%1),0(%2),%[key]\n"
"8: jnz 1b\n"
" j 5f\n"
"3: la %4,255(%1)\n" /* %4 = ptr + 255 */
@@ -192,7 +198,7 @@ static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
" slgr %4,%1\n"
" clgr %0,%4\n" /* copy crosses next page boundary? */
" jnh 6f\n"
- "4: mvcs 0(%4,%1),0(%2),%3\n"
+ "4: mvcs 0(%4,%1),0(%2),%[key]\n"
"9: slgr %0,%4\n"
" j 6f\n"
"5: slgr %0,%0\n"
@@ -200,17 +206,19 @@ static inline unsigned long copy_to_user_mvcs(void __user *ptr, const void *x,
EX_TABLE(0b,3b) EX_TABLE(2b,3b) EX_TABLE(4b,6b)
EX_TABLE(7b,3b) EX_TABLE(8b,3b) EX_TABLE(9b,6b)
: "+a" (size), "+a" (ptr), "+a" (x), "+a" (tmp1), "=a" (tmp2)
- : : "cc", "memory");
+ : [key] "d" (key << 4)
+ : "cc", "memory");
return size;
}
-unsigned long raw_copy_to_user(void __user *to, const void *from, unsigned long n)
+unsigned long raw_copy_to_user_with_key(void __user *to, const void *from,
+ unsigned long n, unsigned long key)
{
if (copy_with_mvcos())
- return copy_to_user_mvcos(to, from, n);
- return copy_to_user_mvcs(to, from, n);
+ return copy_to_user_mvcos(to, from, n, (u8)key);
+ return copy_to_user_mvcs(to, from, n, (u8)key);
}
-EXPORT_SYMBOL(raw_copy_to_user);
+EXPORT_SYMBOL(raw_copy_to_user_with_key);
static inline unsigned long clear_user_mvcos(void __user *to, unsigned long size)
{
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index ac0394087f7d..adce966edb7a 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -114,6 +114,20 @@ __copy_from_user(void *to, const void __user *from, unsigned long n)
return raw_copy_from_user(to, from, n);
}
+#ifdef raw_copy_from_user_with_key
+static __always_inline __must_check unsigned long
+__copy_from_user_with_key(void *to, const void __user *from, unsigned long n,
+ unsigned long key)
+{
+ might_fault();
+ if (should_fail_usercopy())
+ return n;
+ instrument_copy_from_user(to, from, n);
+ check_object_size(to, n, false);
+ return raw_copy_from_user_with_key(to, from, n, key);
+}
+#endif /* raw_copy_from_user_with_key */
+
/**
* __copy_to_user_inatomic: - Copy a block of data into user space, with less checking.
* @to: Destination address, in user space.
@@ -148,6 +162,20 @@ __copy_to_user(void __user *to, const void *from, unsigned long n)
return raw_copy_to_user(to, from, n);
}
+#ifdef raw_copy_to_user_with_key
+static __always_inline __must_check unsigned long
+__copy_to_user_with_key(void __user *to, const void *from, unsigned long n,
+ unsigned long key)
+{
+ might_fault();
+ if (should_fail_usercopy())
+ return n;
+ instrument_copy_to_user(to, from, n);
+ check_object_size(from, n, true);
+ return raw_copy_to_user_with_key(to, from, n, key);
+}
+#endif /* raw_copy_to_user_with_key */
+
#ifdef INLINE_COPY_FROM_USER
static inline __must_check unsigned long
_copy_from_user(void *to, const void __user *from, unsigned long n)
base-commit: bad13799e0305deb258372b7298a86be4c78aaba
prerequisite-patch-id: 5f8ae41bde2fa5717a775e17c08239ed1ddbcc83
--
2.32.0
On Mon, Jan 24, 2022 at 11:38:12AM +0100, Janis Schoetterl-Glausch wrote: > KVM on s390 needs a mechanism to do accesses to guest memory > that honors storage key protection. > __copy_from/to_user_with_key is implemented by introducing > raw_copy_from/to_user_with_key. > Since the existing uaccess implementation on s390 makes use of move > instructions that support having an additional access key supplied, > we can implement raw_copy_from/to_user_with_key by enhancing the > existing implementation. > > Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> > --- > > This works for us and compiles on other architectures (tested x86). > The patch only implements __copy_from/to_user_with_key, since those > are the ones we actually need. On other architectures those functions > don't exists, but they aren't used either, so it's not a problem. Adding an API where only underscored function names are to be used can be considered suboptimal. > Should we also implement single and no underscore variants? Why? > Completeness? Please make this _fully_ symmetrical to the existing copy_to/from_user() implementations, like I tried to say several times. Maybe I wasn't clear enough about this. Also the default implementation - that is if an architecture makes use of copy_to_user_key() without providing a raw_copy_from_user_key() implementation - should fallback to regular copy_to_user() semantics, like I tried to outline with the ifndef example of raw_copy_from_user_key() previously. Furthermore this should be splitted into two patches: one which adds the common code infrastructure, like described above; and a second patch which adds the actual s390 architecture backend/override. The patches should contain a _detailed_ description why the first patch, aka API, should probably be in common code (staying in sync with code instrumentation, etc.); and of course it should contain enough information for people not familiar with s390's storage keys so they can figure out what this is about. Hopefully we get some feedback and either this is acceptable for common code one way or the other, or we have to maintain this on our own, and get the additional maintenance cost for free. Please make sure to add Al Viro, Kees Cook, Arnd Bergmann, and Andrew Morton to cc on your next version, so we hopefully come to a conclusion and can move on.
On 1/24/22 18:41, Heiko Carstens wrote: > On Mon, Jan 24, 2022 at 11:38:12AM +0100, Janis Schoetterl-Glausch wrote: >> KVM on s390 needs a mechanism to do accesses to guest memory >> that honors storage key protection. >> __copy_from/to_user_with_key is implemented by introducing >> raw_copy_from/to_user_with_key. >> Since the existing uaccess implementation on s390 makes use of move >> instructions that support having an additional access key supplied, >> we can implement raw_copy_from/to_user_with_key by enhancing the >> existing implementation. >> >> Signed-off-by: Janis Schoetterl-Glausch <scgl@linux.ibm.com> >> --- >> >> This works for us and compiles on other architectures (tested x86). >> The patch only implements __copy_from/to_user_with_key, since those >> are the ones we actually need. On other architectures those functions >> don't exists, but they aren't used either, so it's not a problem. > > Adding an API where only underscored function names are to be used can be > considered suboptimal. > >> Should we also implement single and no underscore variants? Why? >> Completeness? > > Please make this _fully_ symmetrical to the existing copy_to/from_user() > implementations, like I tried to say several times. Maybe I wasn't clear > enough about this. Also the default implementation - that is if an > architecture makes use of copy_to_user_key() without providing a > raw_copy_from_user_key() implementation - should fallback to regular > copy_to_user() semantics, like I tried to outline with the ifndef example > of raw_copy_from_user_key() previously. That does help. One thing I'm still confused about is the rational for the default implementation. Are you suggesting that copy_from/to_user be implemented in terms of copy_from/to_user_with_key? I didn't think so, even tho you said something along those lines, because I assumed you were referring to the architecture specific implementations for copy_from/to_user, since we weren't talking about common code changes back then and Christian's suggestion didn't feature it either. When you say "fully symmetrical" do you mean all functions that wrap architecture defined access to user space: __copy_from_user_inatomic __copy_from_user __copy_to_user_inatomic __copy_to_user _copy_from_user _copy_to_user copy_from_user copy_to_user __copy_from_user_inatomic_nocache copy_struct_from_user copy_from_user_nofault copy_to_user_nofault strncpy_from_user_nofault strnlen_user_nofault > > Furthermore this should be splitted into two patches: one which adds the > common code infrastructure, like described above; and a second patch which > adds the actual s390 architecture backend/override. > > The patches should contain a _detailed_ description why the first patch, > aka API, should probably be in common code (staying in sync with code > instrumentation, etc.); and of course it should contain enough information > for people not familiar with s390's storage keys so they can figure out > what this is about. > > Hopefully we get some feedback and either this is acceptable for common > code one way or the other, or we have to maintain this on our own, and get > the additional maintenance cost for free. > > Please make sure to add Al Viro, Kees Cook, Arnd Bergmann, and Andrew > Morton to cc on your next version, so we hopefully come to a conclusion and > can move on. Thanks, will do.
© 2016 - 2026 Red Hat, Inc.