Re: [PATCH] initity: try to improve __nocapture annotations
From: Kees Cook
Date: Wed Feb 01 2017 - 19:40:29 EST
On Wed, Feb 1, 2017 at 8:11 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> There are some additional declarations that got missed in the original patch,
> and some annotated functions that use the pointer is a correct but nonobvious
> way:
>
> mm/kasan/kasan.c: In function 'memmove':
> mm/kasan/kasan.c:346:7: error: 'memmove' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
> void *memmove(void *dest, const void *src, size_t len)
> ^~~~~~~
> mm/kasan/kasan.c: In function 'memcpy':
> mm/kasan/kasan.c:355:7: error: 'memcpy' captures its 2 ('src') parameter, please remove it from the nocapture attribute. [-Werror]
> void *memcpy(void *dest, const void *src, size_t len)
> ^~~~~~
> drivers/acpi/acpica/utdebug.c: In function 'acpi_debug_print':
> drivers/acpi/acpica/utdebug.c:158:1: error: 'acpi_debug_print' captures its 3 ('function_name') parameter, please remove it from the nocapture attribute. [-Werror]
>
> lib/string.c:893:7: error: 'memchr_inv' captures its 1 ('start') parameter, please remove it from the nocapture attribute. [-Werror]
> void *memchr_inv(const void *start, int c, size_t bytes)
> lib/string.c: In function 'strnstr':
> lib/string.c:832:7: error: 'strnstr' captures its 1 ('s1') parameter, please remove it from the nocapture attribute. [-Werror]
> char *strnstr(const char *s1, const char *s2, size_t len)
> ^~~~~~~
> lib/string.c:832:7: error: 'strnstr' captures its 2 ('s2') parameter, please remove it from the nocapture attribute. [-Werror]
>
> I'm not sure if these are all appropriate fixes, please have a careful look
>
> Fixes: c2bc07665495 ("initify: Mark functions with the __nocapture attribute")
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxx>
> ---
> drivers/acpi/acpica/utdebug.c | 2 +-
> include/acpi/acpixf.h | 2 +-
> include/asm-generic/asm-prototypes.h | 8 ++++----
> include/linux/string.h | 2 +-
> lib/string.c | 2 +-
> mm/kasan/kasan.c | 4 ++--
> 6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/acpi/acpica/utdebug.c b/drivers/acpi/acpica/utdebug.c
> index 044df9b0356e..de3c9cb305a2 100644
> --- a/drivers/acpi/acpica/utdebug.c
> +++ b/drivers/acpi/acpica/utdebug.c
> @@ -154,7 +154,7 @@ static const char *acpi_ut_trim_function_name(const char *function_name)
> *
> ******************************************************************************/
>
> -void ACPI_INTERNAL_VAR_XFACE
> +void __unverified_nocapture(3) ACPI_INTERNAL_VAR_XFACE
> acpi_debug_print(u32 requested_debug_level,
> u32 line_number,
> const char *function_name,
This might be better by marking acpi_ut_trim_function_name() as
__nocapture. I'll give it a try...
> diff --git a/include/acpi/acpixf.h b/include/acpi/acpixf.h
> index 9f4637e9dd92..9644cec5b082 100644
> --- a/include/acpi/acpixf.h
> +++ b/include/acpi/acpixf.h
> @@ -946,7 +946,7 @@ ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
> const char *module_name,
> u32 component_id,
> const char *format, ...))
> -ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6)
> +ACPI_DBG_DEPENDENT_RETURN_VOID(ACPI_PRINTF_LIKE(6) __nocapture(3)
> void ACPI_INTERNAL_VAR_XFACE
> acpi_debug_print_raw(u32 requested_debug_level,
> u32 line_number,
I wonder why the plugin needs this at all: function_name (the third
arg) isn't even used in the function.
> diff --git a/include/asm-generic/asm-prototypes.h b/include/asm-generic/asm-prototypes.h
> index 939869c772b1..ffc0dd7e8ed2 100644
> --- a/include/asm-generic/asm-prototypes.h
> +++ b/include/asm-generic/asm-prototypes.h
> @@ -2,12 +2,12 @@
> #undef __memset
> extern void *__memset(void *, int, __kernel_size_t);
> #undef __memcpy
> -extern void *__memcpy(void *, const void *, __kernel_size_t);
> +extern void *__memcpy(void *, const void *, __kernel_size_t) __nocapture(2);
> #undef __memmove
> -extern void *__memmove(void *, const void *, __kernel_size_t);
> +extern void *__memmove(void *, const void *, __kernel_size_t) __nocapture(2);
> #undef memset
> extern void *memset(void *, int, __kernel_size_t);
> #undef memcpy
> -extern void *memcpy(void *, const void *, __kernel_size_t);
> +extern void *memcpy(void *, const void *, __kernel_size_t) __nocapture(2);
> #undef memmove
> -extern void *memmove(void *, const void *, __kernel_size_t);
> +extern void *memmove(void *, const void *, __kernel_size_t) __nocapture(2);
Ah yeah, KASAN. This seems correct.
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 8b3b97e7b2b0..0ee877593464 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -76,7 +76,7 @@ static inline __must_check char *strstrip(char *str)
> extern char * strstr(const char *, const char *) __nocapture(-1, 2);
> #endif
> #ifndef __HAVE_ARCH_STRNSTR
> -extern char * strnstr(const char *, const char *, size_t) __nocapture(-1, 2);
> +extern char * strnstr(const char *, const char *, size_t);
That doesn't seem right: strnstr doesn't capture...
> #endif
> #ifndef __HAVE_ARCH_STRLEN
> extern __kernel_size_t strlen(const char *) __nocapture(1);
> diff --git a/lib/string.c b/lib/string.c
> index ed83562a53ae..01151a1a0b61 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -870,7 +870,7 @@ void *memchr(const void *s, int c, size_t n)
> EXPORT_SYMBOL(memchr);
> #endif
>
> -static void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
> +static __always_inline void *check_bytes8(const u8 *start, u8 value, unsigned int bytes)
Is this from another fix? Seems unrelated?
> {
> while (bytes) {
> if (*start != value)
> diff --git a/mm/kasan/kasan.c b/mm/kasan/kasan.c
> index 5f6e09c88d25..ebc02ee1118e 100644
> --- a/mm/kasan/kasan.c
> +++ b/mm/kasan/kasan.c
> @@ -343,7 +343,7 @@ void *memset(void *addr, int c, size_t len)
> }
>
> #undef memmove
> -void *memmove(void *dest, const void *src, size_t len)
> +__unverified_nocapture(2) void *memmove(void *dest, const void *src, size_t len)
> {
> check_memory_region((unsigned long)src, len, false, _RET_IP_);
> check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> @@ -352,7 +352,7 @@ void *memmove(void *dest, const void *src, size_t len)
> }
>
> #undef memcpy
> -void *memcpy(void *dest, const void *src, size_t len)
> +__unverified_nocapture(2) void *memcpy(void *dest, const void *src, size_t len)
> {
> check_memory_region((unsigned long)src, len, false, _RET_IP_);
> check_memory_region((unsigned long)dest, len, true, _RET_IP_);
> --
> 2.9.0
>
Thanks for the patch! I'll try to reproduce the warnings and get some
fixes built if Emese or PaX Team don't beat me to it. :)
-Kees
--
Kees Cook
Pixel Security