LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Nicholas Piggin <npiggin@gmail.com>, linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed
Date: Fri, 27 Mar 2020 08:21:49 +0100
Message-ID: <633fbf5b-8227-1640-f056-7e5d203895dd@c-s.fr> (raw)
In-Reply-To: <20200327070240.427074-3-npiggin@gmail.com>



Le 27/03/2020 à 08:02, Nicholas Piggin a écrit :
> get/put_user can be called with nontrivial arguments. fs/proc/page.c
> has a good example:
> 
>      if (put_user(stable_page_flags(ppage), out)) {
> 
> stable_page_flags is quite a lot of code, including spin locks in the
> page allocator.
> 
> Ensure these arguments are evaluated before user access is allowed.
> This improves security by reducing code with access to userspace, but
> it also fixes a PREEMPT bug with KUAP on powerpc/64s:
> stable_page_flags is currently called with AMR set to allow writes,
> it ends up calling spin_unlock(), which can call preempt_schedule. But
> the task switch code can not be called with AMR set (it relies on
> interrupts saving the register), so this blows up.
> 
> It's fine if the code inside allow_user_access is preemptible, because
> a timer or IPI will save the AMR, but it's not okay to explicitly
> cause a reschedule.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/include/asm/uaccess.h | 97 ++++++++++++++++++------------
>   1 file changed, 59 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/uaccess.h b/arch/powerpc/include/asm/uaccess.h
> index 670910df3cc7..1cf8595aeef1 100644
> --- a/arch/powerpc/include/asm/uaccess.h
> +++ b/arch/powerpc/include/asm/uaccess.h
> @@ -162,36 +162,48 @@ do {								\
>   	prevent_write_to_user(ptr, size);			\
>   } while (0)
>   
> -#define __put_user_nocheck(x, ptr, size, do_allow)			\
> +#define __put_user_nocheck(x, ptr, size, do_allow)		\

No need to touch this line. Anyway at the end, you still have several \ 
which are not aligned.

>   ({								\
>   	long __pu_err;						\
>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> +	__typeof__(*(ptr)) __pu_val = (x);			\
> +	__typeof__(size) __pu_size = (size);			\
> +								\
>   	if (!is_kernel_addr((unsigned long)__pu_addr))		\
>   		might_fault();					\
> -	__chk_user_ptr(ptr);					\
> -	if (do_allow)								\

No need to touch that line

> -		__put_user_size((x), __pu_addr, (size), __pu_err);		\
> -	else									\

No need to touch that line

> -		__put_user_size_allowed((x), __pu_addr, (size), __pu_err);	\
> +	__chk_user_ptr(__pu_addr);				\
> +	if (do_allow)						\
> +		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +	else							\
> +		__put_user_size_allowed(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +								\
>   	__pu_err;						\
>   })
>   
> -#define __put_user_check(x, ptr, size)					\
> -({									\
> -	long __pu_err = -EFAULT;					\
> -	__typeof__(*(ptr)) __user *__pu_addr = (ptr);			\
> -	might_fault();							\
> -	if (access_ok(__pu_addr, size))			\
> -		__put_user_size((x), __pu_addr, (size), __pu_err);	\
> -	__pu_err;							\

Same comment applies, you are touching some lines just to change the \, 
but at the end you still have some misaligned ones.

It would help the review not to touch unchanged lines just for that.

Same comment applies a few places below as well.

> +#define __put_user_check(x, ptr, size)				\
> +({								\
> +	long __pu_err = -EFAULT;				\
> +	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> +	__typeof__(*(ptr)) __pu_val = (x);			\
> +	__typeof__(size) __pu_size = (size);			\
> +								\
> +	might_fault();						\
> +	if (access_ok(__pu_addr, __pu_size))			\
> +		__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +								\
> +	__pu_err;						\
>   })
>   
>   #define __put_user_nosleep(x, ptr, size)			\
>   ({								\
>   	long __pu_err;						\
>   	__typeof__(*(ptr)) __user *__pu_addr = (ptr);		\
> -	__chk_user_ptr(ptr);					\
> -	__put_user_size((x), __pu_addr, (size), __pu_err);	\
> +	__typeof__(*(ptr)) __pu_val = (x);			\
> +	__typeof__(size) __pu_size = (size);			\
> +								\
> +	__chk_user_ptr(__pu_addr);				\
> +	__put_user_size(__pu_val, __pu_addr, __pu_size, __pu_err); \
> +								\
>   	__pu_err;						\
>   })
>   
> @@ -278,46 +290,55 @@ do {								\
>   #define __long_type(x) \
>   	__typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
>   
> -#define __get_user_nocheck(x, ptr, size, do_allow)			\
> +#define __get_user_nocheck(x, ptr, size, do_allow)		\
>   ({								\
>   	long __gu_err;						\
>   	__long_type(*(ptr)) __gu_val;				\
> -	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
> -	__chk_user_ptr(ptr);					\
> +	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
> +	__typeof__(size) __gu_size = (size);			\
> +								\
> +	__chk_user_ptr(__gu_addr);				\
>   	if (!is_kernel_addr((unsigned long)__gu_addr))		\
>   		might_fault();					\
>   	barrier_nospec();					\
> -	if (do_allow)								\
> -		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);		\
> -	else									\
> -		__get_user_size_allowed(__gu_val, __gu_addr, (size), __gu_err);	\
> +	if (do_allow)						\
> +		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	else							\
> +		__get_user_size_allowed(__gu_val, __gu_addr, __gu_size, __gu_err); \
>   	(x) = (__typeof__(*(ptr)))__gu_val;			\
> +								\
>   	__gu_err;						\
>   })
>   
> -#define __get_user_check(x, ptr, size)					\
> -({									\
> -	long __gu_err = -EFAULT;					\
> -	__long_type(*(ptr)) __gu_val = 0;				\
> +#define __get_user_check(x, ptr, size)				\
> +({								\
> +	long __gu_err = -EFAULT;				\
> +	__long_type(*(ptr)) __gu_val = 0;			\
>   	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
> -	might_fault();							\
> -	if (access_ok(__gu_addr, (size))) {		\
> -		barrier_nospec();					\
> -		__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
> -	}								\
> -	(x) = (__force __typeof__(*(ptr)))__gu_val;				\
> -	__gu_err;							\
> +	__typeof__(size) __gu_size = (size);			\
> +								\
> +	might_fault();						\
> +	if (access_ok(__gu_addr, __gu_size)) {			\
> +		barrier_nospec();				\
> +		__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	}							\
> +	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
> +								\
> +	__gu_err;						\
>   })
>   
>   #define __get_user_nosleep(x, ptr, size)			\
>   ({								\
>   	long __gu_err;						\
>   	__long_type(*(ptr)) __gu_val;				\
> -	__typeof__(*(ptr)) __user *__gu_addr = (ptr);	\
> -	__chk_user_ptr(ptr);					\
> +	__typeof__(*(ptr)) __user *__gu_addr = (ptr);		\
> +	__typeof__(size) __gu_size = (size);			\
> +								\
> +	__chk_user_ptr(__gu_addr);				\
>   	barrier_nospec();					\
> -	__get_user_size(__gu_val, __gu_addr, (size), __gu_err);	\
> -	(x) = (__force __typeof__(*(ptr)))__gu_val;			\
> +	__get_user_size(__gu_val, __gu_addr, __gu_size, __gu_err); \
> +	(x) = (__force __typeof__(*(ptr)))__gu_val;		\
> +								\
>   	__gu_err;						\
>   })
>   
> 


Christophe

  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-03-27  7:02 [PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Nicholas Piggin
2020-03-27  7:02 ` [PATCH 2/4] powerpc/64s: use mmu_has_feature in set_kuap() and get_kuap() Nicholas Piggin
2020-03-27  7:02 ` [PATCH 3/4] powerpc/uaccess: evaluate macro arguments once, before user access is allowed Nicholas Piggin
2020-03-27  7:21   ` Christophe Leroy [this message]
2020-03-27  7:02 ` [PATCH 4/4] powerpc/uaccess: add more __builtin_expect annotations Nicholas Piggin
2020-03-27  7:13 ` [PATCH 1/4] powerpc/64s: implement probe_kernel_read/write without touching AMR Christophe Leroy

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=633fbf5b-8227-1640-f056-7e5d203895dd@c-s.fr \
    --to=christophe.leroy@c-s.fr \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=npiggin@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git