Re: [PATCH 1/2] mm/usercopy: get rid of "provably correct" warnings

[Kees Cook]

On Tue, Aug 23, 2016 at 3:28 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> With CONFIG_DEBUG_STRICT_USER_COPY_CHECKS=y, if I force enable the
> __compiletime_object_size() macro with a recent compiler by removing the
> "GCC_VERSION < 40600" check, I get a bunch of false positive warnings.
> For example:
>
>   In function âcopy_to_user.part.8â,
>       inlined from âcopy_to_userâ,
>       inlined from âproc_put_longâ at /home/jpoimboe/git/linux/kernel/sysctl.c:2096:6:
>   /home/jpoimboe/git/linux/arch/x86/include/asm/uaccess.h:723:46: warning: call to â__copy_to_user_overflowâ declared with attribute warning: copy_to_user() buffer size is not provably correct
>
> The problem is that gcc can't always definitively tell whether
> copy_from_user_overflow() will be called.  And when in doubt, it prints
> the warning anyway.  So in practice, these warnings mostly just create a
> lot of noise.  There might be a bug lurking in there somewhere, but the
> signal to noise ratio is really low, and not worth the pain IMO.
>
> So just remove the "provably correct" warnings altogether.  This also
> lays the groundwork for re-enabling the copy_from_user_overflow()
> runtime warnings for newer compilers.

Hrrrm, I'd much rather split configs or something. This "probably
correct" warning is something gcc should be ABLE to do, but the
ability regressed and hasn't yet been fixed:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=46639
originally: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48880

We should get that back at some point, and I'd like to have the
compile-time checks enabled again then without having to reintroduce
the code.

Jeff, any news on this front? It'd be really nice to get this back in.
One of your comments in 2014 on the bug make it sound like it might be
easy to fix?

-Kees

>
> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> ---
>  arch/parisc/include/asm/uaccess.h |  8 +-------
>  arch/s390/include/asm/uaccess.h   |  6 +-----
>  arch/tile/include/asm/uaccess.h   |  3 +--
>  arch/x86/include/asm/uaccess.h    | 35 -----------------------------------
>  4 files changed, 3 insertions(+), 49 deletions(-)
>
> diff --git a/arch/parisc/include/asm/uaccess.h b/arch/parisc/include/asm/uaccess.h
> index 0f59fd9..b34c022 100644
> --- a/arch/parisc/include/asm/uaccess.h
> +++ b/arch/parisc/include/asm/uaccess.h
> @@ -208,13 +208,7 @@ unsigned long copy_in_user(void __user *dst, const void __user *src, unsigned lo
>  #define __copy_to_user_inatomic __copy_to_user
>  #define __copy_from_user_inatomic __copy_from_user
>
> -extern void copy_from_user_overflow(void)
> -#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> -        __compiletime_error("copy_from_user() buffer size is not provably correct")
> -#else
> -        __compiletime_warning("copy_from_user() buffer size is not provably correct")
> -#endif
> -;
> +extern void copy_from_user_overflow(void);
>
>  static inline unsigned long __must_check copy_from_user(void *to,
>                                            const void __user *from,
> diff --git a/arch/s390/include/asm/uaccess.h b/arch/s390/include/asm/uaccess.h
> index 9b49cf1..6d36860 100644
> --- a/arch/s390/include/asm/uaccess.h
> +++ b/arch/s390/include/asm/uaccess.h
> @@ -332,11 +332,7 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>         return __copy_to_user(to, from, n);
>  }
>
> -void copy_from_user_overflow(void)
> -#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> -__compiletime_warning("copy_from_user() buffer size is not provably correct")
> -#endif
> -;
> +void copy_from_user_overflow(void);
>
>  /**
>   * copy_from_user: - Copy a block of data from user space.
> diff --git a/arch/tile/include/asm/uaccess.h b/arch/tile/include/asm/uaccess.h
> index 0a9c4265..e0e313f 100644
> --- a/arch/tile/include/asm/uaccess.h
> +++ b/arch/tile/include/asm/uaccess.h
> @@ -422,8 +422,7 @@ _copy_from_user(void *to, const void __user *from, unsigned long n)
>   * option is not really compatible with -Werror, which is more useful in
>   * general.
>   */
> -extern void copy_from_user_overflow(void)
> -       __compiletime_warning("copy_from_user() size is not provably correct");
> +extern void copy_from_user_overflow(void);
>
>  static inline unsigned long __must_check copy_from_user(void *to,
>                                           const void __user *from,
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index a0ae610..89c12cb 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -710,20 +710,6 @@ copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
>
>  #undef copy_user_diag
>
> -#ifdef CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
> -
> -extern void
> -__compiletime_warning("copy_from_user() buffer size is not provably correct")
> -__copy_from_user_overflow(void) __asm__("copy_from_user_overflow");
> -#define __copy_from_user_overflow(size, count) __copy_from_user_overflow()
> -
> -extern void
> -__compiletime_warning("copy_to_user() buffer size is not provably correct")
> -__copy_to_user_overflow(void) __asm__("copy_from_user_overflow");
> -#define __copy_to_user_overflow(size, count) __copy_to_user_overflow()
> -
> -#else
> -
>  static inline void
>  __copy_from_user_overflow(int size, unsigned long count)
>  {
> @@ -732,8 +718,6 @@ __copy_from_user_overflow(int size, unsigned long count)
>
>  #define __copy_to_user_overflow __copy_from_user_overflow
>
> -#endif
> -
>  static inline unsigned long __must_check
>  copy_from_user(void *to, const void __user *from, unsigned long n)
>  {
> @@ -743,24 +727,6 @@ copy_from_user(void *to, const void __user *from, unsigned long n)
>
>         kasan_check_write(to, n);
>
> -       /*
> -        * While we would like to have the compiler do the checking for us
> -        * even in the non-constant size case, any false positives there are
> -        * a problem (especially when DEBUG_STRICT_USER_COPY_CHECKS, but even
> -        * without - the [hopefully] dangerous looking nature of the warning
> -        * would make people go look at the respecitive call sites over and
> -        * over again just to find that there's no problem).
> -        *
> -        * And there are cases where it's just not realistic for the compiler
> -        * to prove the count to be in range. For example when multiple call
> -        * sites of a helper function - perhaps in different source files -
> -        * all doing proper range checking, yet the helper function not doing
> -        * so again.
> -        *
> -        * Therefore limit the compile time checking to the constant size
> -        * case, and do only runtime checking for non-constant sizes.
> -        */
> -
>         if (likely(sz < 0 || sz >= n)) {
>                 check_object_size(to, n, false);
>                 n = _copy_from_user(to, from, n);
> @@ -781,7 +747,6 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>
>         might_fault();
>
> -       /* See the comment in copy_from_user() above. */
>         if (likely(sz < 0 || sz >= n)) {
>                 check_object_size(from, n, true);
>                 n = _copy_to_user(to, from, n);
> --
> 2.7.4
>



-- 
Kees Cook
Nexus Security