Skip to contentSkip to conversation

arm64: avoid multiple evaluation of ptr in get_user/put_user()

PATCH New AKASHI Takahiro 2 participants 5 messages Sep 24, 2013 – Sep 25, 2013 arm64

lore
Share & tools
lore

AKASHI Takahiro

author
#0 lore raw

PATCH 1/1 arm64: avoid multiple evaluation of ptr in get_user/put_user()

get_user() is defined as a function macro in arm64, and trace_get_user() calls it as followed:

     get_user(ch, ptr++);

Since the second parameter occurs twice in the definition, 'ptr++' is unexpectedly evaluated twice and trace_get_user() will generate a bogus string from user-provided one. As a result, some ftrace sysfs operations, like "echo FUNCNAME > set_ftrace_filter," hit this case and eventually fail. This patch fixes the issue both in get_user() and put_user().

Signed-off-by AKASHI Takahiro <[email protected]>
---
  arch/arm64/include/asm/uaccess.h |   12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +4 -2
index edb3d5c..bbeab83 100644
@@ -166,9 +166,11 @@ do {									\
   #define get_user(x, ptr)						\
  ({									\
+	__typeof__(*(ptr)) *optr = (ptr);				\
+									\
  	might_fault();							\
-	access_ok(VERIFY_READ, (ptr), sizeof(*(ptr))) ?			\
-		__get_user((x), (ptr)) :				\
+	access_ok(VERIFY_READ, optr, sizeof(*optr)) ?			\
+		__get_user((x), optr) :					\
  		((x) = 0, -EFAULT);					\
  })
  @@ -227,9 +229,11 @@ do {									\
   #define put_user(x, ptr)						\

({ \
+ __typeof__(*(ptr)) *optr = (ptr); \
+ \
might_fault(); \
- access_ok(VERIFY_WRITE, (ptr), sizeof(*(ptr))) ? \
- __put_user((x), (ptr)) : \
+ access_ok(VERIFY_WRITE, optr, sizeof(*optr)) ? \
+ __put_user((x), optr) : \
-EFAULT; \
})
-- 1.7.9.5

maintainer
#1 lore raw

PATCH 1/1 arm64: avoid multiple evaluation of ptr in get_user/put_user()

Quoted 19 lines · AKASHI Takahiro

get_user() is defined as a function macro in arm64, and trace_get_user() calls it as followed:

     get_user(ch, ptr++);

Since the second parameter occurs twice in the definition, 'ptr++' is unexpectedly evaluated twice and trace_get_user() will generate a bogus string from user-provided one. As a result, some ftrace sysfs operations, like "echo FUNCNAME > set_ftrace_filter," hit this case and eventually fail. This patch fixes the issue both in get_user() and put_user().

Signed-off-by AKASHI Takahiro <[email protected]>
---
  arch/arm64/include/asm/uaccess.h |   12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +0 -0
index edb3d5c..bbeab83 100644

arch/arm64/include/asm/uaccess.h do { \ · line 168

Quoted 3 lines
arch/arm64/include/asm/uaccess.h +1 -0
@@ -166,9 +166,11 @@ do {									\
   #define get_user(x, ptr)						\
  ({									\
+	__typeof__(*(ptr)) *optr = (ptr);				\

I think this should be:

	__typeof__(*(ptr)) __user *optr = (ptr);

Otherwise the patch looks fine. I can fix the above.

Thanks.

Signature · 1 line
Catalin
maintainer
#2 lore raw

PATCH 1/1 arm64: avoid multiple evaluation of ptr in get_user/put_user()

In reply to AKASHI Takahiro
Quoted 33 lines · AKASHI Takahiro

get_user() is defined as a function macro in arm64, and trace_get_user() calls it as followed:

     get_user(ch, ptr++);

Since the second parameter occurs twice in the definition, 'ptr++' is unexpectedly evaluated twice and trace_get_user() will generate a bogus string from user-provided one. As a result, some ftrace sysfs operations, like "echo FUNCNAME > set_ftrace_filter," hit this case and eventually fail. This patch fixes the issue both in get_user() and put_user().

Signed-off-by AKASHI Takahiro <[email protected]>
---
  arch/arm64/include/asm/uaccess.h |   12 ++++++++----
  1 file changed, 8 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +4 -2
index edb3d5c..bbeab83 100644
@@ -166,9 +166,11 @@ do {									\
   #define get_user(x, ptr)						\
  ({									\
+	__typeof__(*(ptr)) *optr = (ptr);				\
+									\
  	might_fault();							\
-	access_ok(VERIFY_READ, (ptr), sizeof(*(ptr))) ?			\
-		__get_user((x), (ptr)) :				\
+	access_ok(VERIFY_READ, optr, sizeof(*optr)) ?			\
+		__get_user((x), optr) :					\
  		((x) = 0, -EFAULT);					\
  })
  @@ -227,9 +229,11 @@ do {									\
   #define put_user(x, ptr)						\

  ({									\

arch/arm64/include/asm/uaccess.h

Quoted 10 lines
arch/arm64/include/asm/uaccess.h +4 -2
+	__typeof__(*(ptr)) *optr = (ptr);				\
+									\
  	might_fault();							\
-	access_ok(VERIFY_WRITE, (ptr), sizeof(*(ptr))) ?		\
-		__put_user((x), (ptr)) :				\
+	access_ok(VERIFY_WRITE, optr, sizeof(*optr)) ?			\
+		__put_user((x), optr) :					\
  		-EFAULT;						\
  })
  -- 1.7.9.5

BTW, please use git send-email or other email client, the diff above is heavily corrupted (too many spaces at the beginning of the line, removed empty lines; I managed to fix it up this time but only because it was a small patch).

Signature · 1 line
Catalin
author
#3 lore raw

PATCH 1/1 arm64: avoid multiple evaluation of ptr in get_user/put_user()

Catalin,

Thank you for your comments, and let me send a revised patch again because my original message didn't reach MLs due to my screw-up.

BTW, I will submit a patch of ftrace support on arm64 once gcc supports gprof (-pg) options.

-Takahiro AKASHI

Quoted 50 lines · Catalin Marinas
Quoted 33 lines · AKASHI Takahiro

get_user() is defined as a function macro in arm64, and trace_get_user() calls it as followed:

      get_user(ch, ptr++);

Since the second parameter occurs twice in the definition, 'ptr++' is unexpectedly evaluated twice and trace_get_user() will generate a bogus string from user-provided one. As a result, some ftrace sysfs operations, like "echo FUNCNAME > set_ftrace_filter," hit this case and eventually fail. This patch fixes the issue both in get_user() and put_user().

Signed-off-by AKASHI Takahiro <[email protected]>
---
   arch/arm64/include/asm/uaccess.h |   12 ++++++++----
   1 file changed, 8 insertions(+), 4 deletions(-)
arch/arm64/include/asm/uaccess.h +4 -2
index edb3d5c..bbeab83 100644
@@ -166,9 +166,11 @@ do {									\
    #define get_user(x, ptr)						\
   ({									\
+	__typeof__(*(ptr)) *optr = (ptr);				\
+									\
   	might_fault();							\
-	access_ok(VERIFY_READ, (ptr), sizeof(*(ptr))) ?			\
-		__get_user((x), (ptr)) :				\
+	access_ok(VERIFY_READ, optr, sizeof(*optr)) ?			\
+		__get_user((x), optr) :					\
   		((x) = 0, -EFAULT);					\
   })
   @@ -227,9 +229,11 @@ do {									\
    #define put_user(x, ptr)						\

   ({									\

arch/arm64/include/asm/uaccess.h

Quoted 10 lines
arch/arm64/include/asm/uaccess.h +4 -2
+	__typeof__(*(ptr)) *optr = (ptr);				\
+									\
   	might_fault();							\
-	access_ok(VERIFY_WRITE, (ptr), sizeof(*(ptr))) ?		\
-		__put_user((x), (ptr)) :				\
+	access_ok(VERIFY_WRITE, optr, sizeof(*optr)) ?			\
+		__put_user((x), optr) :					\
   		-EFAULT;						\
   })
   -- 1.7.9.5

BTW, please use git send-email or other email client, the diff above is heavily corrupted (too many spaces at the beginning of the line, removed empty lines; I managed to fix it up this time but only because it was a small patch).

maintainer
#4 lore raw

PATCH 1/1 arm64: avoid multiple evaluation of ptr in get_user/put_user()

Quoted 2 lines · AKASHI Takahiro

Thank you for your comments, and let me send a revised patch again because my original message didn't reach MLs due to my screw-up.

You can send it if you want but I already merged it in my tree (should appear in -next this week as well).

Signature · 1 line
Catalin