Re: [PATCH] initity: try to improve __nocapture annotations

[Arnd Bergmann]

On Thu, Feb 2, 2017 at 1:39 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Wed, Feb 1, 2017 at 8:11 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:

>> -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...

I tried that without success: the problem is actually the later acpi_os_printf()
that takes the result from acpi_ut_trim_function_name().

>> 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.

This one wasn't needed, I should probably have left it out. It just seemed
right for consistency to do the same for acpi_debug_print_raw() and
acpi_debug_print().

>> 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...

Right, so better __unverified_nocapture() then?

>>  #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?

This fixed a warning about memchr_inv() for me: when check_bytes8()
is inlined, the compiler can see that the argument is not captured, but
if gcc decides against inlining, it warns.

>> 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. :)

I later got more warnings for some of the string functions, but we can
deal with them
separately.

    Arnd